Skip to content

Commit

Permalink
Fix accidentally quadratic behavior in _modify
Browse files Browse the repository at this point in the history
We need to be careful to not grab an extra reference when mutating data
structures because that means we make extra copies.  Doing that every
time in `_modify` is really painful, as that function implements `|=`
and all modify-assign operators.

`jv_setpath()` also grabs additional references, so this is not the only
fix needed for the modify-assign operators to not be accidentally
quadratic.

We have to use `LOADVN` in order to make this fix possible, so we should
really byte-code `_modify` rather than jq-code it.  However, as a
stop-gap to make this fix easier, I'm adding syntax for referring to a
`$binding` such that you get `LOADVN` instead of `LOADV`.

This syntax is not meant to be used outside jq's internals!  Therefore
it is not documented.  I promise to break it later, so don't use it!

TBD, but later:

 - Optimize `_modify` for the case where `update` outputs more than one
   value.

   This is trivial: add a `setpath($p; null)` in the middle of `_modify`
   before calling `update`.  But unfortunately the VM retains a
   reference to `value_at_path` for path expression checking, and fixing
   that will require more work.
  • Loading branch information
nicowilliams committed Oct 24, 2021
1 parent 0c3455d commit a9f97e9
Show file tree
Hide file tree
Showing 4 changed files with 1,122 additions and 1,102 deletions.
20 changes: 19 additions & 1 deletion src/builtin.jq
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,25 @@ def min_by(f): _min_by_impl(map([f]));
def add: reduce .[] as $x (null; . + $x);
def del(f): delpaths([path(f)]);
def _assign(paths; $value): reduce path(paths) as $p (.; setpath($p; $value));
def _modify(paths; update): reduce path(paths) as $p (.; label $out | (setpath($p; getpath($p) | update) | ., break $out), delpaths([$p]));
def _modify(paths; update):
reduce path(paths) as $p (.;
. as $dot
| null
| label $out
| ($dot | getpath($p)) as $v
| (
( $$$$v
| update
| (., break $out) as $v
| $$$$dot
| setpath($p; $v)
),
(
$$$$dot
| delpaths([$p])
)
)
);
def map_values(f): .[] |= f;

# recurse
Expand Down
Loading

0 comments on commit a9f97e9

Please sign in to comment.