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

Optimization error in update operation #499

Closed
wants to merge 2 commits into from
Closed

Optimization error in update operation #499

wants to merge 2 commits into from

Conversation

anthony-redFox
Copy link
Contributor

No description provided.

@anthony-redFox anthony-redFox changed the title Optimization on error on update operation Optimization error in update operation Dec 29, 2017
@@ -23,6 +23,10 @@ function err(operation, expectedTarget, path) {
);
}

const tr = true;
const arrayMethods = {push: tr, unshift: tr, concat: tr, splice: tr, pop: tr, shift: tr};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be nice to use Set.
@Yomguithereal what do you think about drop IE9 and IE10?

@Zache
Copy link
Contributor

Zache commented Dec 29, 2017 via email

@anthony-redFox
Copy link
Contributor Author

@Zache please don't, what?

@Zache
Copy link
Contributor

Zache commented Dec 30, 2017 via email

@anthony-redFox
Copy link
Contributor Author

anthony-redFox commented Dec 30, 2017

@Zache Why do I ask?

  1. Microsoft doesn't support IE9 and IE10 at all
  2. Most libraries like lodash, mocha already drop or will drop IE9 and IE10.
  3. I am working on enterprise project (PLM system for big companies) and we don't support IE9 and IE10

Can you explain, why IE9 or IE10 still works for you?

@Yomguithereal
Copy link
Owner

Hello @anthony-redFox. I don't really see the point of your PR. What are you trying to fix or enhance here?

@anthony-redFox
Copy link
Contributor Author

Hello, @Yomguithereal it is just optimization of throw a error and as result the bundle will be less size.

@Yomguithereal
Copy link
Owner

@anthony-redFox. Your edit should produce more checks and conditions than before. I am not sure it improves performance. Since you test the existence of a string as an object key now.

@anthony-redFox
Copy link
Contributor Author

I agree with you that I added additional check.

Also by benchmark nothing changed
Without patch:

Baobab#get x 589,771 ops/sec ±0.76% (94 runs sampled)
Baobab#set x 185,282 ops/sec ±0.31% (97 runs sampled)
Baobab#unset x 849,941 ops/sec ±0.75% (95 runs sampled)
Baobab.Cursor#set x 38,957 ops/sec ±4.22% (89 runs sampled)
Baobab.Cursor#unset x 815,053 ops/sec ±0.36% (93 runs sampled)
Baobab.Cursor#push x 1,082 ops/sec ±44.72% (10 runs sampled)
Baobab.Cursor#unshift x 464 ops/sec ±1.39% (84 runs sampled)
Baobab.Cursor#splice x 538 ops/sec ±1.73% (89 runs sampled)

With patch

Baobab#get x 595,541 ops/sec ±0.73% (95 runs sampled)
Baobab#set x 183,132 ops/sec ±0.46% (94 runs sampled)
Baobab#unset x 833,419 ops/sec ±2.08% (91 runs sampled)
Baobab.Cursor#set x 38,183 ops/sec ±4.43% (89 runs sampled)
Baobab.Cursor#unset x 829,665 ops/sec ±0.51% (95 runs sampled)
Baobab.Cursor#push x 1,034 ops/sec ±40.79% (10 runs sampled)
Baobab.Cursor#unshift x 474 ops/sec ±1.36% (82 runs sampled)
Baobab.Cursor#splice x 557 ops/sec ±0.34% (93 runs sampled)

If you still disagree with me, please close PR.

@Yomguithereal
Copy link
Owner

Difference seems negligible and could be due to other factors affecting the engine outside of our control. No?

@anthony-redFox
Copy link
Contributor Author

Yes, right.

@Yomguithereal
Copy link
Owner

Ok. I will close this then. Thanks for the contribution anyway @anthony-redFox.

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.

3 participants