url: make the context non-enumerable

[joyeecheung]

At the moment we expose the context as a normal property on the URL
which makes them enumerable and considered
by assert libraries even though the context carries path-dependent
information that do not affect the equivalence of these objects.
This patch fixes it in a minimal manner by marking the context
non-enumerable as making it full private would require more
refactoring and can be done in a bigger patch later.

Refs: #24211

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[nodejs-github-bot]

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1524/pipeline

[joyeecheung]

CI: https://ci.nodejs.org/job/node-test-pull-request/18398/

[joyeecheung]

Forgot the URL#href reuse, sorry. Fixed. Another CI: https://ci.nodejs.org/job/node-test-pull-request/18399/

Can you take another look? @lpinca

[Trott]

Landed in 5e1bf05

[TimothyGu]

@joyeecheung Did you confirm that there was no performance regression? I remember the main reason this wasn't done in the beginning was that Object.defineProperty had abysmal performance compared to just a simple assignment.

[joyeecheung]

@TimothyGu I benchmarked a bit and indeed there is about a ~10% regression, not a whole lot but still there is some overhead.

Taking benchmark results with the modified benchmarks in #24302

                                                                                         confidence improvement accuracy (*)   (**)   (***)
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='auth' withBase='false'              ***    -13.11 %       ±2.28% ±3.03%  ±3.95%
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='auth' withBase='true'               ***    -13.50 %       ±4.08% ±5.42%  ±7.06%
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='dot' withBase='false'               ***    -17.76 %       ±3.49% ±4.69%  ±6.21%
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='dot' withBase='true'                ***    -12.67 %       ±4.74% ±6.33%  ±8.26%
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='file' withBase='false'              ***    -14.93 %       ±2.15% ±2.86%  ±3.72%
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='file' withBase='true'               ***    -12.85 %       ±3.44% ±4.58%  ±5.99%
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='idn' withBase='false'               ***    -12.99 %       ±1.66% ±2.21%  ±2.88%
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='idn' withBase='true'                ***    -14.30 %       ±2.41% ±3.21%  ±4.19%
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='javascript' withBase='false'        ***    -16.15 %       ±4.14% ±5.51%  ±7.19%
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='javascript' withBase='true'         ***    -15.01 %       ±1.98% ±2.64%  ±3.44%
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='long' withBase='false'              ***     -9.14 %       ±2.45% ±3.27%  ±4.28%
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='long' withBase='true'                **     -5.69 %       ±3.80% ±5.10%  ±6.72%
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='percent' withBase='false'           ***    -15.92 %       ±3.31% ±4.41%  ±5.74%
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='percent' withBase='true'             **    -10.49 %       ±6.14% ±8.25% ±10.91%
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='short' withBase='false'             ***    -13.54 %       ±2.83% ±3.78%  ±4.94%
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='short' withBase='true'              ***    -13.32 %       ±3.44% ±4.60%  ±6.01%
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='wpt' withBase='false'               ***    -15.02 %       ±3.53% ±4.73%  ±6.20%
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='wpt' withBase='true'                ***    -10.09 %       ±3.60% ±4.80%  ±6.27%
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='ws' withBase='false'                ***    -16.88 %       ±3.18% ±4.23%  ±5.51%
 url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='ws' withBase='true'                 ***    -11.45 %       ±4.72% ±6.30%  ±8.25%

The impact is a bit smaller in create-clientrequest.js which benchmarks the path in http.ClientRequest that parses URL strings to options with new URL

                                                          confidence improvement accuracy (*)   (**)  (***)
 http/create-clientrequest.js e=1 arg='string' url='idn'         ***     -5.56 %       ±1.86% ±2.45% ±3.15%
 http/create-clientrequest.js e=1 arg='string' url='long'        ***     -3.02 %       ±1.30% ±1.71% ±2.20%
 http/create-clientrequest.js e=1 arg='string' url='wpt'         ***     -4.04 %       ±1.99% ±2.62% ±3.37%

On a side note there is an even bigger regression when I tested out a prototype with WeakMaps (on the old benchmark)

                                                                              confidence improvement accuracy (*)   (**)  (***)
 url/legacy-vs-whatwg-url-parse.js n=100000 method='whatwg' type='auth'              ***    -49.83 %       ±1.33% ±1.77% ±2.32%
 url/legacy-vs-whatwg-url-parse.js n=100000 method='whatwg' type='dot'               ***    -43.16 %       ±1.59% ±2.12% ±2.77%
 url/legacy-vs-whatwg-url-parse.js n=100000 method='whatwg' type='file'              ***    -48.49 %       ±1.91% ±2.55% ±3.34%
 url/legacy-vs-whatwg-url-parse.js n=100000 method='whatwg' type='idn'               ***    -37.16 %       ±1.82% ±2.43% ±3.19%
 url/legacy-vs-whatwg-url-parse.js n=100000 method='whatwg' type='javascript'        ***    -57.77 %       ±1.25% ±1.66% ±2.17%
 url/legacy-vs-whatwg-url-parse.js n=100000 method='whatwg' type='long'              ***    -25.12 %       ±1.31% ±1.75% ±2.29%
 url/legacy-vs-whatwg-url-parse.js n=100000 method='whatwg' type='percent'           ***    -44.16 %       ±1.93% ±2.58% ±3.38%
 url/legacy-vs-whatwg-url-parse.js n=100000 method='whatwg' type='short'             ***    -44.17 %       ±1.73% ±2.31% ±3.01%
 url/legacy-vs-whatwg-url-parse.js n=100000 method='whatwg' type='ws'                ***    -43.94 %       ±2.34% ±3.13% ±4.12%

[Trott]

Is the performance hit worth reverting or adding do-not-land-on- labels? Is the hope that subsequently making it fully private will improve performance?

[TimothyGu]

I think the performance hit warrants a revert. My original plan was to wait until private class fields come out, which would hopefully improve the performance.

[BridgeAR]

Performance tracking bug: https://bugs.chromium.org/p/v8/issues/detail?id=8447

[joyeecheung]

I think the performance hit warrants a revert.

I guess that depends on which one is more important: regression with assert libraries or regression in performance? (10+% of performance hit isn't really that huge, IMO)

My original plan was to wait until private class fields come out, which would hopefully improve the performance.

I tried with that idea (with --harmony-private-fields) but there are several issues:

• We need to refactor the implementation because private fields at this point are only accessible in the class body - so, not accessible in parse callbacks, unless we make them private methods. Many of the URL methods and accessors are also currently defined outside of class bodies to play nice with WebIDL, there's probably some way to get away with this but I didn't look further to see if there is a way to do it with a small diff.
• The current private fields implementation jumps through several hoops ATM and I am not sure when the optimized-enough version will be landed here. The bottleneck here seems to be the call to the runtime C++ function ObjectDefineProperty which is certainly slower than StaNamedProperty . Private fields are currently implemented with more runtime calls and it'll take a while to optimize all that (BTW I am implementing private methods in the upstream so I more or less know about the existing hoops). If we need to refactor everything out with the class features, it will take a while for the overall performance to catch up in v8 (although TBH I think that would make the URL class much more readable even when we need to sacrifice performance).

(There is an alternative of using v8::Private but as the comment of it says, that's experimental, you'll need to use it at your own risk..also I am not quite sure if calling into C++ to use it is any faster than calling into ObjectDefineProperty)

[TimothyGu]

To fix #24211, we could just set flags correctly. Creating a PR right now.

[joyeecheung]

@TimothyGu SGTM. While you are at it, I remember there was an interesting perf boost in the href setter with this patch, probably because we now reuse the existing URLContext in parse()instead of doing url[context] = new URLContext() all the time, so it's probably worth keeping that branch (although, I think parse() can even be inlined in the two call sites now that there isn't much to share).