build: allow enabling the --trace-maps flag in V8 #14018
Conversation
nodejs-github-bot
added
the
build
|
Context please? |
node.gyp
Outdated
| @@ -1,6 +1,7 @@ | |||
| { | |||
| 'variables': { | |||
| 'v8_use_snapshot%': 'false', | |||
| 'v8_trace_maps%': '0', | |||
configure
Outdated
| parser.add_option('--enable-trace-maps', | ||
| action='store_true', | ||
| dest='trace_maps', | ||
| help='Enable the --trace-maps flag in v8') |
There was a problem hiding this comment.
Consider undocumenting this or add a (use at your own risk) because it introduces an incompatibility between builds with and without the flag.
It adds a field to SharedFunctionInfo that ends up in the code cache so the result of new vm.Script(source, { produceCachedData: true }) from a -DTRACE_MAPS build won't be loadable in a normal build and vice versa. I didn't test but I expect it segfaults.
There was a problem hiding this comment.
I'm +1 to documenting with a warning. Quite a few of configure's switches have wide reaching results...
|
@refack It's a tool for tracing hidden class transitions. ('hidden class' == 'map' in V8 parlance.) |
|
Updated, PTAL |
configure
Outdated
| parser.add_option('--enable-trace-maps', | ||
| action='store_true', | ||
| dest='trace_maps', | ||
| help='Enable the --trace-maps flag in v8 (use at your own risk)') |
There was a problem hiding this comment.
Actually, one nit: V8 with a capital V, since we started disambiguating due to Node (v)8 ;)
This can be useful for tracing map creation. PR-URL: nodejs#14018 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com>
|
Landed in 49d13a1. Thanks! |
|
@evanlucas This seems to conflict with the snapshot change, can you backport to v8.x? |
This can be useful for tracing map creation. PR-URL: nodejs#14018 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com>
This can be useful for tracing map creation. Backport-PR-URL: #14344 Backport-Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Backport-Reviewed-By: Refael Ackermann <refack@gmail.com> PR-URL: #14018 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com>
This can be useful for tracing map creation. Backport-PR-URL: #14344 Backport-Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Backport-Reviewed-By: Refael Ackermann <refack@gmail.com> PR-URL: #14018 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com>
|
Should this have been Should we backport to lts? |
|
ping @evanlucas |
|
@MylesBorins sorry I somehow missed your previous comment. I think it could be argued either way that this is semver-minor. It isn't exposed in release builds and can only be done when using |
|
https://chromium-review.googlesource.com/c/v8/v8/+/734648 -- |
|
I've landed on v6.x. Please lmk if there are any concerns |
This can be useful for tracing map creation. PR-URL: #14018 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
build
This can be useful for tracing map creation.