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.

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

Fix bug where objects are converted to '[Object object]' in a VM #255

Closed

Conversation

markdalgleish
Copy link
Contributor

Fixes #240

I added a test case that fails against the original implementation.

This comes with a bit of a perf hit but unfortunately there's no way around it if you want to reliably detect the native toString function. I made a JSBench so you can see the difference: https://jsbench.me/1xkpq8eozj

@dcousens
Copy link
Collaborator

dcousens commented Jun 10, 2021

I think we might have to languish in the fact that we added this feature knowing that this could be a risk.
No free lunch.

Thanks for this pull request @markdalgleish , I'll try it out as soon as I can 👍

@juangl
Copy link

juangl commented Feb 14, 2022

hey, guys, wondering if there's anything blocking this from being merged.

@dcousens
Copy link
Collaborator

@juangl as first step, we probably need a benchmark to compare this against the existing code - some questions might appear if the benchmark is severely impacted

@ericbarrionuevo
Copy link

@juangl as first step, we probably need a benchmark to compare this against the existing code - some questions might appear if the benchmark is severely impacted

I agree with your statement about the existing code

@dcousens
Copy link
Collaborator

dcousens commented Sep 12, 2022

Benchmark output from today

Mozilla/5.0 (X11; Linux x86_64; rv:104.0) Gecko/20100101 Firefox/104.0
* local#strings x 10,290,716 ops/sec ±1.33% (67 runs sampled)
* npm#strings x 10,321,700 ops/sec ±1.40% (67 runs sampled)
* local/dedupe#strings x 2,231,927 ops/sec ±0.33% (68 runs sampled)
* npm/dedupe#strings x 2,251,416 ops/sec ±0.28% (68 runs sampled)

> Fastest is npm#strings

* local#object x 1,764,792 ops/sec ±1.92% (49 runs sampled)
* npm#object x 7,663,848 ops/sec ±1.90% (63 runs sampled)
* local/dedupe#object x 1,620,503 ops/sec ±1.94% (57 runs sampled)
* npm/dedupe#object x 4,105,373 ops/sec ±0.85% (64 runs sampled)

> Fastest is npm#object

* local#strings, object x 1,733,121 ops/sec ±2.83% (33 runs sampled)
* npm#strings, object x 5,691,563 ops/sec ±0.64% (66 runs sampled)
* local/dedupe#strings, object x 1,184,113 ops/sec ±1.60% (61 runs sampled)
* npm/dedupe#strings, object x 2,082,282 ops/sec ±0.23% (67 runs sampled)

> Fastest is npm#strings, object

* local#mix x 624,883 ops/sec ±2.59% (58 runs sampled)
* npm#mix x 3,176,306 ops/sec ±0.69% (67 runs sampled)
* local/dedupe#mix x 432,948 ops/sec ±4.70% (60 runs sampled)
* npm/dedupe#mix x 837,616 ops/sec ±7.09% (62 runs sampled)

> Fastest is npm#mix

* local#arrays x 572,171 ops/sec ±1.72% (57 runs sampled)
* npm#arrays x 1,285,368 ops/sec ±0.73% (64 runs sampled)
* local/dedupe#arrays x 444,003 ops/sec ±1.69% (61 runs sampled)
* npm/dedupe#arrays x 754,877 ops/sec ±0.40% (66 runs sampled)

> Fastest is npm#arrays

Finished

This change has a severe performance impact on the object path, with performance dropping by half, down to a fifth in some instances.

I don't know if that trade-off is ecofriendly for running this in a vm, but I understand the plight.

@dcousens
Copy link
Collaborator

dcousens commented Sep 13, 2022

Merged in #281, with the typical usage performance problems resolved

@dcousens dcousens closed this Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Class names being returned as [object Object] as of v2.3.0
4 participants