Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove hasOwnProperty call #183

Merged
merged 4 commits into from
Oct 8, 2024
Merged

remove hasOwnProperty call #183

merged 4 commits into from
Oct 8, 2024

Conversation

gurgunday
Copy link
Contributor

Since the object no longer has a prototype chain, we can just check if a key exists by accessing it

blakeembrey
blakeembrey reviewed Oct 8, 2024
View reviewed changes
src/index.ts
@@ -121,7 +120,7 @@ export function parse(
const key = str.slice(keyStartIdx, keyEndIdx);

// only assign once
if (!__hasOwnProperty.call(obj, key)) {
if (obj[key] === undefined) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you double check performance? I didn’t change this because my numbers were worse doing the undefined check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, let me bench it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bench LGTM, would you be free to rebase on main and remove the if (value !== undefined) now? Since it won't matter if dec returns undefined from #179.

@gurgunday
Copy link
Contributor Author

On my Linux machine (CPU 12th Gen Intel(R) Core(TM) i9-12900H)

New:

 ✓ src/parse.bench.ts (26) 39576ms
   ✓ parse (6) 7940ms
     name                   hz     min     max    mean     p75     p99    p995    p999     rme  samples
   · simple       9,702,920.99  0.0001  0.2114  0.0001  0.0001  0.0001  0.0002  0.0003  ±0.25%  4851461   fastest
   · decode       4,651,623.54  0.0002  0.0680  0.0002  0.0002  0.0002  0.0003  0.0008  ±0.12%  2325812
   · unquote      9,406,398.95  0.0001  0.7161  0.0001  0.0001  0.0001  0.0002  0.0003  ±0.31%  4703200
   · duplicates   1,929,250.28  0.0004  0.2137  0.0005  0.0005  0.0006  0.0007  0.0015  ±0.15%   964626
   · 10 cookies     827,978.72  0.0010  0.4642  0.0012  0.0012  0.0019  0.0024  0.0081  ±0.67%   413990
   · 100 cookies     66,803.29  0.0128  0.5871  0.0150  0.0146  0.0230  0.0285  0.2132  ±0.75%    33402   slowest
   ✓ parse top-sites (20) 31633ms
     name                                  hz     min     max    mean     p75     p99    p995    p999     rme   samples
   · parse accounts.google.com   7,581,630.32  0.0001  0.0834  0.0001  0.0001  0.0002  0.0002  0.0004  ±0.13%   3790816
   · parse amazon.com            2,568,358.75  0.0003  0.5899  0.0004  0.0004  0.0005  0.0006  0.0011  ±0.59%   1284180
   · parse apple.com             7,825,036.53  0.0001  1.4933  0.0001  0.0001  0.0002  0.0003  0.0013  ±0.63%   3912519
   · parse cloudflare.com        7,231,698.58  0.0001  0.3224  0.0001  0.0001  0.0002  0.0003  0.0009  ±0.61%   3615850
   · parse docs.google.com       7,349,305.38  0.0001  0.1290  0.0001  0.0001  0.0002  0.0002  0.0003  ±0.14%   3674653
   · parse drive.google.com      7,018,752.67  0.0001  0.2117  0.0001  0.0001  0.0002  0.0002  0.0003  ±0.17%   3509377
   · parse en.wikipedia.org      2,102,850.00  0.0004  0.0437  0.0005  0.0005  0.0005  0.0006  0.0010  ±0.07%   1051425
   · parse istockphoto.com         536,774.79  0.0015  0.6531  0.0019  0.0019  0.0022  0.0025  0.0112  ±0.73%    268388   slowest
   · parse maps.google.com       7,355,024.25  0.0001  4.1665  0.0001  0.0001  0.0002  0.0002  0.0006  ±1.77%   3677513
   · parse play.google.com       7,561,780.41  0.0001  0.5561  0.0001  0.0001  0.0002  0.0002  0.0005  ±0.60%   3780891
   · parse policies.google.com   7,608,549.95  0.0001  0.5061  0.0001  0.0001  0.0002  0.0002  0.0005  ±0.77%   3804275
   · parse pt.wikipedia.org      2,670,298.36  0.0003  0.0561  0.0004  0.0004  0.0004  0.0005  0.0010  ±0.12%   1335150
   · parse sites.google.com      7,172,833.14  0.0001  0.2123  0.0001  0.0001  0.0002  0.0002  0.0007  ±0.15%   3586417
   · parse support.google.com    4,458,951.07  0.0002  0.7200  0.0002  0.0002  0.0003  0.0003  0.0008  ±0.97%   2229476
   · parse t.me                  7,294,171.24  0.0001  0.9658  0.0001  0.0001  0.0002  0.0002  0.0003  ±0.42%   3647086
   · parse vk.com                  853,759.58  0.0010  0.3518  0.0012  0.0012  0.0013  0.0017  0.0041  ±0.52%    426964
   · parse www.google.com        4,583,636.97  0.0002  0.5562  0.0002  0.0002  0.0003  0.0003  0.0007  ±0.55%   2291819
   · parse youtu.be              1,877,702.66  0.0005  0.2178  0.0005  0.0005  0.0006  0.0006  0.0014  ±0.16%    938852
   · parse youtube.com           1,901,106.15  0.0005  0.2301  0.0005  0.0005  0.0006  0.0006  0.0012  ±0.16%    950554
   · parse example.com          22,561,632.11  0.0000  0.2071  0.0000  0.0000  0.0001  0.0001  0.0001  ±0.17%  11280817   fastest

Old (master / 0f56c6e):

 ✓ src/parse.bench.ts (26) 40091ms
   ✓ parse (6) 7787ms
     name                   hz     min     max    mean     p75     p99    p995    p999     rme  samples
   · simple       8,727,527.23  0.0001  1.0084  0.0001  0.0001  0.0001  0.0002  0.0003  ±0.57%  4363764   fastest
   · decode       4,127,381.38  0.0002  0.0453  0.0002  0.0002  0.0003  0.0004  0.0005  ±0.11%  2063691
   · unquote      8,228,980.06  0.0001  0.2382  0.0001  0.0001  0.0001  0.0002  0.0003  ±0.20%  4114491
   · duplicates   2,007,016.13  0.0004  1.5755  0.0005  0.0005  0.0006  0.0007  0.0022  ±0.76%  1003509
   · 10 cookies     868,530.59  0.0010  0.6681  0.0012  0.0011  0.0014  0.0017  0.0066  ±0.47%   434266
   · 100 cookies     68,317.46  0.0124  1.4316  0.0146  0.0142  0.0251  0.0383  0.1045  ±0.68%    34159   slowest
   ✓ parse top-sites (20) 32303ms
     name                                  hz     min     max    mean     p75     p99    p995    p999     rme   samples
   · parse accounts.google.com   7,279,393.53  0.0001  0.0678  0.0001  0.0001  0.0002  0.0002  0.0003  ±0.15%   3639697
   · parse amazon.com            2,534,713.80  0.0003  0.3295  0.0004  0.0004  0.0005  0.0006  0.0011  ±0.40%   1267357
   · parse apple.com             8,430,491.56  0.0001  0.0868  0.0001  0.0001  0.0002  0.0002  0.0003  ±0.13%   4215246
   · parse cloudflare.com        8,000,964.86  0.0001  3.6636  0.0001  0.0001  0.0001  0.0002  0.0005  ±1.55%   4000483
   · parse docs.google.com       7,647,191.59  0.0001  0.2127  0.0001  0.0001  0.0002  0.0002  0.0003  ±0.15%   3823596
   · parse drive.google.com      7,698,992.83  0.0001  0.0532  0.0001  0.0001  0.0002  0.0002  0.0003  ±0.14%   3849497
   · parse en.wikipedia.org      2,146,053.02  0.0004  0.5728  0.0005  0.0005  0.0006  0.0006  0.0010  ±0.35%   1073027
   · parse istockphoto.com         550,595.79  0.0015  0.6311  0.0018  0.0018  0.0027  0.0030  0.0102  ±0.88%    275298   slowest
   · parse maps.google.com       7,597,900.78  0.0001  5.0688  0.0001  0.0001  0.0002  0.0003  0.0006  ±2.12%   3798951
   · parse play.google.com       8,055,075.47  0.0001  0.7307  0.0001  0.0001  0.0002  0.0002  0.0008  ±0.66%   4027538
   · parse policies.google.com   7,681,858.95  0.0001  0.4727  0.0001  0.0001  0.0002  0.0002  0.0005  ±0.82%   3840933
   · parse pt.wikipedia.org      2,568,763.73  0.0003  0.4443  0.0004  0.0004  0.0006  0.0007  0.0009  ±0.21%   1284382
   · parse sites.google.com      7,427,749.51  0.0001  0.2212  0.0001  0.0001  0.0002  0.0002  0.0003  ±0.14%   3713875
   · parse support.google.com    4,725,513.75  0.0002  0.6865  0.0002  0.0002  0.0003  0.0003  0.0008  ±1.12%   2362757
   · parse t.me                  7,036,034.16  0.0001  0.9734  0.0001  0.0001  0.0002  0.0002  0.0009  ±0.43%   3518018
   · parse vk.com                  900,729.42  0.0010  0.3716  0.0011  0.0011  0.0013  0.0017  0.0053  ±0.51%    450365
   · parse www.google.com        4,584,557.85  0.0002  0.5046  0.0002  0.0002  0.0003  0.0003  0.0006  ±0.74%   2292279
   · parse youtu.be              1,931,114.60  0.0004  0.2125  0.0005  0.0005  0.0006  0.0006  0.0016  ±0.17%    965558
   · parse youtube.com           1,945,369.56  0.0005  0.2069  0.0005  0.0005  0.0006  0.0006  0.0014  ±0.14%    972685
   · parse example.com          22,165,850.27  0.0000  0.2121  0.0000  0.0000  0.0001  0.0001  0.0001  ±0.19%  11082926   fastest

@NewEraCracker
Copy link

I strongly recommend against this change.

https://stackoverflow.com/a/46425266 &
https://stackoverflow.com/a/12017703

Please keep the __hasOwnProperty proper check which you introduced with version 0.7.2

Otherwise I will be forced to create another patch-package patch to revert this insecure change back to the secure code.

Thank you.

@blakeembrey
Copy link
Member

blakeembrey commented Oct 8, 2024 •
edited
Loading

@NewEraCracker I think you misunderstand this change. It used to be an undefined check in 0.6 and this is also a null prototype now.

@gurgunday
Copy link
Contributor Author

@NewEraCracker the object in question doesn't have a prototype chain

cookie/src/index.ts

Line 98 in 0f56c6e

const obj: Record<string, string> = new NullObject();

blakeembrey
blakeembrey reviewed Oct 8, 2024
View reviewed changes
src/index.ts Outdated Show resolved Hide resolved
@blakeembrey blakeembrey merged commit 8f3ee9e into jshttp:master Oct 8, 2024
2 of 5 checks passed
@gurgunday gurgunday deleted the remove-hasown branch October 8, 2024 19:13
@blakeembrey
Copy link
Member

blakeembrey commented Oct 8, 2024 •
edited
Loading

Before:

     name                   hz     min     max    mean     p75     p99    p995    p999     rme  samples
   · simple       9,536,666.74  0.0000  1.9262  0.0001  0.0001  0.0001  0.0002  0.0003  ±1.15%  4768334   fastest
   · decode       4,351,025.10  0.0001  0.1439  0.0002  0.0003  0.0003  0.0003  0.0004  ±0.11%  2175513
   · unquote      8,798,691.91  0.0000  3.5239  0.0001  0.0001  0.0002  0.0002  0.0003  ±1.60%  4399346
   · duplicates   1,870,817.76  0.0004  0.1379  0.0005  0.0005  0.0007  0.0008  0.0025  ±0.11%   935409
   · 10 cookies     725,326.87  0.0012  0.2622  0.0014  0.0014  0.0016  0.0018  0.0047  ±0.43%   362664
   · 100 cookies     64,256.19  0.0141  0.2938  0.0156  0.0153  0.0211  0.0258  0.1795  ±0.55%    32129   slowest
   ✓ parse top-sites (20) 30438ms
     name                                  hz     min     max    mean     p75     p99    p995    p999     rme   samples
   · parse accounts.google.com   7,973,679.12  0.0000  3.5668  0.0001  0.0001  0.0002  0.0002  0.0003  ±1.40%   3986840
   · parse amazon.com            2,524,065.60  0.0002  2.8802  0.0004  0.0004  0.0005  0.0005  0.0010  ±1.17%   1262033
   · parse apple.com             8,738,064.27  0.0000  0.2993  0.0001  0.0001  0.0001  0.0002  0.0003  ±0.78%   4369033
   · parse cloudflare.com        7,969,211.55  0.0000  0.4134  0.0001  0.0001  0.0002  0.0002  0.0004  ±0.76%   3984606
   · parse docs.google.com       8,069,740.05  0.0000  0.0374  0.0001  0.0001  0.0002  0.0002  0.0003  ±0.07%   4034871
   · parse drive.google.com      7,891,565.75  0.0000  0.0435  0.0001  0.0001  0.0002  0.0002  0.0003  ±0.07%   3945783
   · parse en.wikipedia.org      2,029,437.43  0.0004  0.3934  0.0005  0.0005  0.0006  0.0007  0.0008  ±0.24%   1014719
   · parse istockphoto.com         411,196.45  0.0022  0.5065  0.0024  0.0024  0.0028  0.0031  0.0075  ±0.71%    205599   slowest
   · parse maps.google.com       7,976,809.60  0.0000  0.6668  0.0001  0.0001  0.0002  0.0002  0.0004  ±0.90%   3988405
   · parse play.google.com       7,898,624.74  0.0000  0.4017  0.0001  0.0001  0.0002  0.0002  0.0005  ±0.79%   3949313
   · parse policies.google.com   7,843,192.89  0.0000  0.4424  0.0001  0.0001  0.0002  0.0002  0.0005  ±1.01%   3921597
   · parse pt.wikipedia.org      2,416,569.74  0.0003  0.0569  0.0004  0.0004  0.0005  0.0005  0.0006  ±0.08%   1208285
   · parse sites.google.com      7,973,319.04  0.0000  0.0571  0.0001  0.0001  0.0002  0.0002  0.0003  ±0.08%   3986660
   · parse support.google.com    4,496,789.40  0.0001  0.6247  0.0002  0.0002  0.0003  0.0003  0.0006  ±1.28%   2248395
   · parse t.me                  7,887,270.60  0.0000  2.3432  0.0001  0.0001  0.0002  0.0002  0.0003  ±0.92%   3943636
   · parse vk.com                  856,894.20  0.0010  0.3931  0.0012  0.0012  0.0014  0.0015  0.0044  ±0.54%    428448
   · parse www.google.com        4,694,536.25  0.0001  0.4127  0.0002  0.0002  0.0003  0.0003  0.0005  ±0.67%   2347269
   · parse youtu.be              1,806,772.16  0.0004  0.0569  0.0006  0.0005  0.0007  0.0007  0.0008  ±0.08%    903389
   · parse youtube.com           1,713,205.32  0.0004  0.0720  0.0006  0.0006  0.0007  0.0008  0.0010  ±0.10%    856603
   · parse example.com          21,520,975.05  0.0000  0.0454  0.0000  0.0000  0.0001  0.0001  0.0001  ±0.07%  10760488   fastest

After:

     name                   hz     min     max    mean     p75     p99    p995    p999     rme  samples
   · simple       9,375,902.65  0.0000  2.1365  0.0001  0.0001  0.0001  0.0002  0.0003  ±1.26%  4687952   fastest
   · decode       4,361,141.90  0.0001  0.0906  0.0002  0.0003  0.0003  0.0003  0.0004  ±0.09%  2180571
   · unquote      9,053,984.48  0.0000  0.3696  0.0001  0.0001  0.0002  0.0002  0.0003  ±0.50%  4526993
   · duplicates   1,899,611.85  0.0004  0.2430  0.0005  0.0005  0.0007  0.0007  0.0008  ±0.16%   949806
   · 10 cookies     733,253.49  0.0011  0.4528  0.0014  0.0013  0.0017  0.0019  0.0058  ±0.78%   366627
   · 100 cookies     67,354.93  0.0131  0.4508  0.0148  0.0144  0.0203  0.0268  0.3113  ±0.92%    33678   slowest
   ✓ parse top-sites (20) 30506ms
     name                                  hz     min     max    mean     p75     p99    p995    p999     rme   samples
   · parse accounts.google.com   8,168,533.30  0.0000  0.3649  0.0001  0.0001  0.0002  0.0002  0.0003  ±0.16%   4084267
   · parse amazon.com            2,504,860.65  0.0003  0.5039  0.0004  0.0004  0.0005  0.0005  0.0010  ±0.72%   1252431
   · parse apple.com             8,366,634.68  0.0000  0.7055  0.0001  0.0001  0.0002  0.0002  0.0003  ±1.40%   4183318
   · parse cloudflare.com        7,707,217.48  0.0000  0.3580  0.0001  0.0001  0.0002  0.0002  0.0005  ±0.76%   3853609
   · parse docs.google.com       8,066,836.08  0.0000  0.0425  0.0001  0.0001  0.0002  0.0002  0.0003  ±0.08%   4033419
   · parse drive.google.com      8,190,201.41  0.0000  0.0496  0.0001  0.0001  0.0002  0.0002  0.0003  ±0.08%   4095101
   · parse en.wikipedia.org      2,032,568.20  0.0004  0.4090  0.0005  0.0005  0.0006  0.0006  0.0008  ±0.34%   1016285
   · parse istockphoto.com         401,386.78  0.0022  0.5097  0.0025  0.0025  0.0028  0.0032  0.0085  ±0.69%    200694   slowest
   · parse maps.google.com       7,883,794.32  0.0000  0.4968  0.0001  0.0001  0.0002  0.0002  0.0005  ±0.92%   3941919
   · parse play.google.com       7,938,028.54  0.0000  0.3647  0.0001  0.0001  0.0002  0.0002  0.0004  ±0.70%   3969015
   · parse policies.google.com   7,803,352.49  0.0000  0.4558  0.0001  0.0001  0.0002  0.0002  0.0005  ±1.04%   3901677
   · parse pt.wikipedia.org      2,416,136.80  0.0003  0.0569  0.0004  0.0004  0.0005  0.0005  0.0006  ±0.12%   1208069
   · parse sites.google.com      8,140,285.53  0.0000  0.1084  0.0001  0.0001  0.0002  0.0002  0.0003  ±0.08%   4070143
   · parse support.google.com    4,467,599.83  0.0001  9.2805  0.0002  0.0002  0.0003  0.0003  0.0006  ±3.84%   2233800
   · parse t.me                  8,063,400.15  0.0000  2.9553  0.0001  0.0001  0.0002  0.0002  0.0003  ±1.16%   4031701
   · parse vk.com                  838,286.50  0.0010  2.7789  0.0012  0.0012  0.0014  0.0016  0.0048  ±1.13%    419144
   · parse www.google.com        4,756,138.03  0.0001  1.8527  0.0002  0.0002  0.0003  0.0003  0.0005  ±0.98%   2378171
   · parse youtu.be              1,851,767.07  0.0004  0.3175  0.0005  0.0005  0.0006  0.0007  0.0008  ±0.16%    925884
   · parse youtube.com           1,851,129.30  0.0004  0.3191  0.0005  0.0005  0.0007  0.0007  0.0008  ±0.15%    925565
   · parse example.com          21,308,312.47  0.0000  0.0374  0.0000  0.0000  0.0001  0.0001  0.0001  ±0.08%  10654157   fastest

It still appears indifferent for me using node v22.9.0. I also found this site: https://andrew.hedges.name/experiments/in/. It seems like hasOwnProperty is faster for me on Safari and slower on Chrome 🤷 Maybe it's our different node/v8 versions?

Edit: Either way I'm good with using undefined, just wanted to update the benchmarks I have.

@gurgunday
Copy link
Contributor Author

I mean, in theory, === undefined also checks the prototype chain if a key is not found on the object itself, but the object has no proto anyway

The === undefined check has a specific bytecode in V8, so I'd expect it to run faster than a function call, but honestly V8 might be doing anything at this level

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blakeembrey blakeembrey blakeembrey left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gurgunday @NewEraCracker @blakeembrey