fix(3517): super-simplistic fix to avoid costly AST transforms when t…

[janl]

…hey are not needed.

    // Skip lengthy AST transforms if the function passed can be
    // safely wrapped in parenthesis.

I’d also be okay with inventing a new “/* no-ast */” prefix that folks can opt into, say:

  "reduce": "/* no-ast */ function(keys, values, rereduce) {…}"

to be absolutely safe we don’t break anything.

Fixes #3517

[nickva]

Look great, Jan!

Would it be possible to throw together a few test cases (along side some existing ones somewhere( which test case when the function has whitespace before/after, has ; and is already an expression?

[nickva]

I had tried a quick test with a 500k docs view from https://gist.github.com/nickva/f4177e581a5ba275b854b9a5af875d84 and don't seem to recover the speedup yet. (Ignore the sizes the dtavg is the time to query the view in seconds, the script has "autoupdate": False ddoc flag to avoid ken auto-indexing, so all the time should be spent building it from scratch

Main

js-1.8.5

file      active    external  dtavg
18616696  17671120  14988273  29.380
18620792  17677145  14988244  28.480
18628984  17676934  14988360  29.301

js-78

file      active    external  dtavg
18076024  17612956  14984735  46.296
18088312  17614972  14984619  46.416
18092408  17605353  14984677  44.620

js-91

file      active    external  dtavg
18354552  17638764  14986272  36.837
18436472  17653248  14986968  34.450
18428280  17654448  14986852  33.743

With fix/3517-part7 PR

js-1.8.5

18620792  17669797  14988215  28.369
18624888  17677835  14988389  28.598
18665848  17673626  14988853  28.989

js-78 + fix/3517-part7

file      active    external  dtavg
18628984  17684599  14988186  34.203
18620792  17676887  14988186  33.917
18624888  17670314  14988215  33.268

js-91 + fix/3517-part7

file      active    external  dtavg
18628984  17673652  14988244  30.974
18633080  17683572  14988215  30.859
18628984  17680489  14988331  30.344

EDIT: Previously I didn't see the performance improvement and as @janl mentioned in couchdb-dev on Slack it's due to my script using here-docs for the reduce function which has an extra \n in front. Updated the comment with the fixed string. Although we could probably try to strip to \n and other whitespace around it to avoid users falling into the same trap.

[nickva]

After the latest update the speedup is apparent. With js-78 it's about 36-37 seconds and for js-91 it's 30-31 seconds.

[nickva]

+1

[nickva]

Sadly I see make release fail:

Installing CouchDB into rel/couchdb/ ...
==> rel (generate)
ERROR: sh(cp -R /Users/nvatama/asf/rel/../share/server/main-ast-bypass.js "/Users/nvatama/asf/rel/couchdb/share/server/main-ast-bypass.js")failed with return code 1 and the following output:
cp: /Users/nvatama/asf/rel/../share/server/main-ast-bypass.js: No such file or directory

ERROR: Unexpected error: rebar_abort
ERROR: generate failed while processing /Users/nvatama/asf/rel: rebar_abort
make: *** [release] Error 1

This is at commit sha

commit 9cbfd0904b8c68cad24021ac7d0ace6d3722154c (HEAD -> fix/3517-part7, origin/fix/3517-part7)

    docs: document workaround

[nickva]

Wonder if a slightly unpleasant part here could be that an edit in bin/couchdb, which is installed by rpm/deb packages, would make that that file unable to be updated automatically by subsequent package upgrades?

RPM would put updates in.rpmnew files and deb, I think?, stops and prompts the user about what to do. Prompts are easy for desktops, but can be a pain for unattended upgrades.

[janl]

you can do this via env var, you don’t have to edit bin/couchdb — that’s just where you get the original paths that are valid for your system

[big-r81]

For me it works, I tried it from scratch.

git branch -D fix/3517-part7
make clean
make
make release

will result in:

... done

    You can now copy the rel/couchdb directory anywhere on your system.
    Start CouchDB with ./bin/couchdb from within that directory.

Git log:

commit 9cbfd0904b8c68cad24021ac7d0ace6d3722154c (HEAD -> fix/3517-part7, origin/fix/3517-part7)
Author: Jan Lehnardt <[email protected]>
Date:   Mon Dec 12 17:39:34 2022 +0100

    docs: document workaround

[janl]

@nickva good find, I’ll update the release script and docs to match (if the ast-bypass file does not exist for them, they don’t need it)

[janl]

hm, or rather, we should build it in all cases, it just won’t do anything on 1.8.5

fixed