search

AmpersandJS / ampersand-state

Core state management object.
MIT License
141 stars 75 forks source link

Optimization: set #222

Closed STRML closed 8 years ago

STRML commented 8 years ago

Initial creation of a model should be faster due to elimination of _previousAttributes (which calls expensive getAttributes), looping through change array, and unnecessary calls to change events and 'compare' hooks on datatypes, when we know the isEqual check will fail.

I'll make some line notes just to clarify why I made certain changes.

wraithgar commented 8 years ago

I'm +1 on this, thanks for all this optimization work you continue to do.

STRML commented 8 years ago

No worries - for context, we use ampersand-state and ampersand-collections on a performance-sensitive React webapp. We actually deconstruct and reconstruct the models every time an attribute changes (rather than using set) so we have easy reference equality in React's shouldComponentUpdate. For that reason, construction must be as fast as possible.

One could argue we should be using Immutable-JS's Records instead, but after building a project with Immutable-JS I'm just not convinced the unintuitive API and edge cases (oops, made an immutable array of mutable objects) is worth it. So we're using this for now.

I'll continue to profile our app and submit PRs as I find bottlenecks.

wraithgar commented 8 years ago

Looking into the merge conflicts now.

STRML commented 8 years ago

Must be from that PR that was just merged in. I'll rebase.

wraithgar commented 8 years ago

@STRML see #223 for a start, the rebase was a little wonky cause of how git does diffs. Perhaps you'll have better luck in your attempt. Let me know if you need anything.

STRML commented 8 years ago

Part of the issue is that I also was dealing with the setup/teardown of listeners in state's compare handler being pretty wonky.

Trying to run tests now but getting a 429 from bitbucket when trying to install phantomjs (blew away deps earlier). Once I figure out why (obviously shouldn't be ratelimited) I'll force-push and ping you.

STRML commented 8 years ago

:+1: Tests pass, looks like someone added saucelabs integration but didn't add a key, encrypted or otherwise, so I would expect every build to fail until that's figured out.

wraithgar commented 8 years ago

The key is present in master. I rebased this w/ master locally and it's all good

STRML commented 8 years ago

Odd. I see it working locally too. I wonder why Travis barfed on it?

wraithgar commented 8 years ago

I'll pull master and run test-ci just to make sure it all passes on all the browsers and then ship.

Thank you so much for your patience on this one, merge conflicts are always a pain.

STRML commented 8 years ago

No worries, thanks for the fast replies!

Michael Garvin mailto:notifications@github.com January 25, 2016 at 5:15 PM

I'll pull master and run |test-ci| just to make sure it all passes on all the browsers and then ship.

Thank you so much for your patience on this one, merge conflicts are always a pain.

— Reply to this email directly or view it on GitHub https://github.com/AmpersandJS/ampersand-state/pull/222#issuecomment-174724103.

wraithgar commented 8 years ago

published as v4.9.0