From c3f1961e36cd3be2327bd1c56a856c0b2077d52c Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 11 Feb 2018 19:34:12 -0800 Subject: [PATCH] Stop copying out function definition when executing a function This switches function execution from the function's source code to its stored node and pstree. This means we no longer have to re-parse the function every time we execute it. --- src/exec.cpp | 29 +++++++++++--------------- src/parse_execution.cpp | 1 + src/parser.cpp | 45 +++++++++++++++++++++-------------------- src/parser.h | 5 +++-- src/proc.h | 1 + tests/test_cmdsub.err | 4 ++-- 6 files changed, 42 insertions(+), 43 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index e329a9646..b201ab5ba 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -305,15 +305,13 @@ static bool io_transmogrify(const io_chain_t &in_chain, io_chain_t *out_chain, /// Morph an io redirection chain into redirections suitable for passing to eval, call eval, and /// clean up morphed redirections. /// -/// \param def the code to evaluate, or the empty string if none -/// \param statement the statement node to evaluate, or empty -/// \param block_type the type of block to push on evaluation +/// \param parsed_source the parsed source code containing the node to evaluate +/// \param node the node to evaluate /// \param ios the io redirections to be performed on this block -static void internal_exec_helper(parser_t &parser, const wcstring &def, - tnode_t statement, - enum block_type_t block_type, const io_chain_t &ios) { - // If we have a valid statement node, then we must not have a string to execute. - assert(!statement || def.empty()); +template +void internal_exec_helper(parser_t &parser, parsed_source_ref_t parsed_source, tnode_t node, + const io_chain_t &ios) { + assert(parsed_source && node && "exec_helper missing source or without node"); io_chain_t morphed_chain; std::vector opened_fds; @@ -325,11 +323,7 @@ static void internal_exec_helper(parser_t &parser, const wcstring &def, return; } - if (!statement) { - parser.eval(def, morphed_chain, block_type); - } else { - parser.eval_node(statement, morphed_chain, block_type); - } + parser.eval_node(parsed_source, node, morphed_chain, TOP); morphed_chain.clear(); io_cleanup_fds(opened_fds); @@ -798,8 +792,6 @@ void exec_job(parser_t &parser, job_t *j) { break; } - wcstring def; - function_get_definition(func_name, &def); const std::map inherit_vars = function_get_inherit_vars(func_name); @@ -811,7 +803,8 @@ void exec_job(parser_t &parser, job_t *j) { verify_buffer_output(); if (!exec_error) { - internal_exec_helper(parser, def, {}, TOP, process_net_io_chain); + internal_exec_helper(parser, props->parsed_source, props->body_node, + process_net_io_chain); } parser.allow_function(); @@ -824,7 +817,9 @@ void exec_job(parser_t &parser, job_t *j) { verify_buffer_output(); if (!exec_error) { - internal_exec_helper(parser, wcstring(), p->internal_block_node, TOP, + assert(p->block_node_source && p->internal_block_node && + "Process is missing node info"); + internal_exec_helper(parser, p->block_node_source, p->internal_block_node, process_net_io_chain); } break; diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 5a0c10603..abcf2f9a1 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -995,6 +995,7 @@ parse_execution_result_t parse_execution_context_t::populate_block_process( if (errored) return parse_execution_errored; proc->type = INTERNAL_BLOCK_NODE; + proc->block_node_source = pstree; proc->internal_block_node = statement; proc->set_io_chain(process_io_chain); return parse_execution_success; diff --git a/src/parser.cpp b/src/parser.cpp index 9fdade353..9017a120c 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -658,36 +658,19 @@ int parser_t::eval(parsed_source_ref_t ps, const io_chain_t &io, enum block_type return 0; } - // Determine the initial eval level. If this is the first context, it's -1; otherwise it's the - // eval level of the top context. This is sort of wonky because we're stitching together a - // global notion of eval level from these separate objects. A better approach would be some - // profile object that all contexts share, and that tracks the eval levels on its own. - int exec_eval_level = - (execution_contexts.empty() ? -1 : execution_contexts.back()->current_eval_level()); - - // Append to the execution context stack. - execution_contexts.push_back(make_unique(ps, this, exec_eval_level)); - const parse_execution_context_t *ctx = execution_contexts.back().get(); - // Execute the first node. tnode_t start{&ps->tree, &ps->tree.front()}; - this->eval_node(start, io, block_type); - - // Clean up the execution context stack. - assert(!execution_contexts.empty() && execution_contexts.back().get() == ctx); - execution_contexts.pop_back(); + this->eval_node(ps, start, io, block_type); return 0; } template -int parser_t::eval_node(tnode_t node, const io_chain_t &io, enum block_type_t block_type) { +int parser_t::eval_node(parsed_source_ref_t ps, tnode_t node, const io_chain_t &io, + enum block_type_t block_type) { static_assert( std::is_same::value || std::is_same::value, "Unexpected node type"); - parse_execution_context_t *ctx = execution_contexts.back().get(); - assert(ctx != NULL); - CHECK_BLOCK(1); // Handle cancellation requests. If our block stack is currently empty, then we already did @@ -711,16 +694,34 @@ int parser_t::eval_node(tnode_t node, const io_chain_t &io, enum block_type_t // Start it up scope_block_t *scope_block = this->push_block(block_type); + + // Determine the initial eval level. If this is the first context, it's -1; otherwise it's the + // eval level of the top context. This is sort of wonky because we're stitching together a + // global notion of eval level from these separate objects. A better approach would be some + // profile object that all contexts share, and that tracks the eval levels on its own. + int exec_eval_level = + (execution_contexts.empty() ? -1 : execution_contexts.back()->current_eval_level()); + + // Append to the execution context stack. + execution_contexts.push_back(make_unique(ps, this, exec_eval_level)); + parse_execution_context_t *ctx = execution_contexts.back().get(); + int result = ctx->eval_node(node, scope_block, io); this->pop_block(scope_block); + // Clean up the execution context stack. + assert(!execution_contexts.empty() && execution_contexts.back().get() == ctx); + execution_contexts.pop_back(); + job_reap(0); // reap again return result; } // Explicit instantiations. TODO: use overloads instead? -template int parser_t::eval_node(tnode_t, const io_chain_t &io, enum block_type_t block_type); -template int parser_t::eval_node(tnode_t, const io_chain_t &io, enum block_type_t block_type); +template int parser_t::eval_node(parsed_source_ref_t, tnode_t, + const io_chain_t &, enum block_type_t); +template int parser_t::eval_node(parsed_source_ref_t, tnode_t, + const io_chain_t &, enum block_type_t); bool parser_t::detect_errors_in_argument_list(const wcstring &arg_list_src, wcstring *out, const wchar_t *prefix) { diff --git a/src/parser.h b/src/parser.h index 04e36e278..edb51dcf6 100644 --- a/src/parser.h +++ b/src/parser.h @@ -251,10 +251,11 @@ class parser_t { /// Evaluate the parsed source ps. int eval(parsed_source_ref_t ps, const io_chain_t &io, enum block_type_t block_type); - /// Evaluates a node in the topmost execution context. + /// Evaluates a node. /// The node type must be grammar::statement or grammar::job_list. template - int eval_node(tnode_t node, const io_chain_t &io, enum block_type_t block_type); + int eval_node(parsed_source_ref_t ps, tnode_t node, const io_chain_t &io, + enum block_type_t block_type); /// Evaluate line as a list of parameters, i.e. tokenize it and perform parameter expansion and /// cmdsubst execution on the tokens. The output is inserted into output. Errors are ignored. diff --git a/src/proc.h b/src/proc.h index 227b45599..74f8ebe41 100644 --- a/src/proc.h +++ b/src/proc.h @@ -83,6 +83,7 @@ class process_t { /// For internal block processes only, the node offset of the statement. /// This is always either block, ifs, or switchs, never boolean or decorated. + parsed_source_ref_t block_node_source{}; tnode_t internal_block_node{}; /// Sets argv. diff --git a/tests/test_cmdsub.err b/tests/test_cmdsub.err index cc56e58f2..1d39d0ae7 100644 --- a/tests/test_cmdsub.err +++ b/tests/test_cmdsub.err @@ -13,8 +13,8 @@ set b (string repeat -n 512 x) # Command sub over the limit should fail fish: Too much data emitted by command substitution so it was discarded -set -l x (string repeat -n $argv x) - ^ + set -l x (string repeat -n $argv x) + ^ in function 'subme' called on standard input with parameter list '513'