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

Performance #83

Open
deser opened this issue Jan 11, 2018 · 18 comments
Open

Performance #83

deser opened this issue Jan 11, 2018 · 18 comments

Comments

@deser
Copy link

deser commented Jan 11, 2018

Seems imutability-helper far away from Immutable.js when mutating. Are there any benchmarks or comparing imutability-helper with alternatives (seemless-immutable\timm\etc..)?

@kolodny
Copy link
Owner

kolodny commented Jan 14, 2018

I haven't really looked into it. I'd be open to a PR with a benchmark folder though

@deser
Copy link
Author

deser commented Jan 14, 2018

Sounds great!

@eldargab
Copy link

eldargab commented Jul 2, 2018

Seems imutability-helper far away from Immutable.js when mutating.

Of course it is...

There are so many invariant checks and expensive paths, e.g.

    invariant(
      typeof spec === 'object' && spec !== null,
      'update(): You provided an invalid spec to update(). The spec and ' +
      'every included key path must be plain objects containing one of the ' +
      'following commands: %s.',
      Object.keys(commands).join(', ')
    );

JS engines are not yet that smart!

Otherwise, with right implementation, this approach should be faster then anything else for small and mid-size objects.

@linq2js
Copy link

linq2js commented Aug 11, 2018

here is benchmarks of common immutable packages https://github.com/linq2js/immhelper/blob/HEAD/benchmarks-result-03.txt

@hoytech
Copy link

hoytech commented Aug 14, 2018

I made a pull request to add update-immutable to the benchmarks. I copied the immutability-helper version of the benchmarks and just replaced it with update-immutable.

On my machine U-I is around 1.5-2x faster than I-H on most of the object tests. For arrays and the tests where it's bounded on deep-freeze it's basically a wash.

Immutable (immutability-helper)
  Verification: P-PPPP-PP-PPPPP-PP-PPPPPP-PPPPP-PPPP-PPPP-PPPP
  Object: read (x500000): 12 ms
  Object: write (x100000): 382 ms
  Object: deep read (x500000): 6 ms
  Object: deep write (x100000): 448 ms
  Object: very deep read (x500000): 34 ms
  Object: very deep write (x100000): 864 ms
  Object: merge (x100000): 221 ms
  Array: read (x500000): 6 ms
  Array: write (x100000): 21871 ms
  Array: deep read (x500000): 5 ms
  Array: deep write (x100000): 21658 ms
Total elapsed = 63 ms (read) + 45444 ms (write) = 45507 ms.

Immutable (update-immutable)
  Verification: P-PPPP-PP-PPPPP-PP-PPPPPP-PPPPP-PPPP-PPPP-PPPP
  Object: read (x500000): 10 ms
  Object: write (x100000): 125 ms
  Object: deep read (x500000): 6 ms
  Object: deep write (x100000): 202 ms
  Object: very deep read (x500000): 34 ms
  Object: very deep write (x100000): 438 ms
  Object: merge (x100000): 121 ms
  Array: read (x500000): 4 ms
  Array: write (x100000): 22116 ms
  Array: deep read (x500000): 6 ms
  Array: deep write (x100000): 22607 ms
Total elapsed = 60 ms (read) + 45609 ms (write) = 45669 ms.

And:

Immutable (immutability-helper) + deep freeze
  Verification: P-PPPP-PP-PPPPP-PP-PPPPPP-PPPPP-PPPP-PPPP-PPPP
  Object: read (x500000): 11 ms
  Object: write (x100000): 272 ms
  Object: deep read (x500000): 5 ms
  Object: deep write (x100000): 419 ms
  Object: very deep read (x500000): 31 ms
  Object: very deep write (x100000): 892 ms
  Object: merge (x100000): 236 ms
  Array: read (x500000): 5 ms
  Array: write (x100000): 22358 ms
  Array: deep read (x500000): 5 ms
  Array: deep write (x100000): 22344 ms
Total elapsed = 57 ms (read) + 46521 ms (write) = 46578 ms.

Immutable (update-immutable) + deep freeze
  Verification: P-PPPP-PP-PPPPP-PP-PPPPPP-PPPPP-PPPP-PPPP-PPPP
  Object: read (x500000): 11 ms
  Object: write (x100000): 116 ms
  Object: deep read (x500000): 6 ms
  Object: deep write (x100000): 186 ms
  Object: very deep read (x500000): 30 ms
  Object: very deep write (x100000): 416 ms
  Object: merge (x100000): 119 ms
  Array: read (x500000): 5 ms
  Array: write (x100000): 22935 ms
  Array: deep read (x500000): 5 ms
  Array: deep write (x100000): 23881 ms
Total elapsed = 59 ms (read) + 47653 ms (write) = 47712 ms.

@linq2js
Copy link

linq2js commented Aug 14, 2018

@hoytech That's great work, but I dont understand why you said UI 1.5-2x faster than IH, it seems UI slower than IH a little bit

@hoytech
Copy link

hoytech commented Aug 14, 2018

Am I misreading the benchmark?

IH: Object: write (x100000): 382 ms
UI: Object: write (x100000): 125 ms

IH: Object: deep write (x100000): 448 ms
UI: Object: deep write (x100000): 202 ms

IH: Object: merge (x100000): 221 ms
UI: Object: merge (x100000): 121 ms

etc.

@linq2js
Copy link

linq2js commented Aug 14, 2018

@hoytech, i just compare total values haha

@hoytech
Copy link

hoytech commented Aug 15, 2018

Hehe yes I figured that's what you were looking at :)

I think the total is a bit misleading since it's heavily skewed to the longer running tests and the shorter tests get drowned out in the noise, however these tests seem like they may be the most important for many (most?) workloads.

That said, I haven't looked at how your benchmark is implemented in enough detail to know what it is actually testing for, so any extra information you have on that would be helpful.

@linq2js
Copy link

linq2js commented Aug 15, 2018

@hoytech you right
I reviewed your source code and realize that we might get slow if update multiple branches of state tree
for sample:

update(state, {
  a: {
    b: {
      c: {}
    },
    ...manyBProps
  },
  ...manyAProps
  }, { a: { b: { c: { $set: newValue }, $set: newValue  } } });

You use recursive call so it can process one spec at same time
A and B props will be cloned twice so process gets slow down
The performance test does not cover multiple update at once, that is big problem if app state should update many times

@hoytech
Copy link

hoytech commented Aug 15, 2018

Yes that is true: One of the advantages of the react update() system is that the overhead of the recursive traversal and shallow copying can be amortised over a number of simultaneous updates.

@linq2js
Copy link

linq2js commented Aug 15, 2018

What do you think about update() returning promise if there is any updated value is promise ?

@hoytech
Copy link

hoytech commented Aug 15, 2018

Hmm interesting. What do you see as being the use-case for this?

My concern is if this would break backwards-compatibility of the react update() interface.

@linq2js
Copy link

linq2js commented Aug 15, 2018

  1. for sample
    const nextState = await update(state, { todos: ['set', loadTodoFromServer()] }); if(nextState !== state) { this.setState({ ...nextState }); }
    does this make sense ?

  2. we can create new method updateAsync(); to explicit updating returns Promise obj
    and we also add more optiong like configure({ asyncUpdate: true }) to enable async mode for default update() func

@hoytech
Copy link

hoytech commented Aug 15, 2018

I think I understand, but just to clarify, what is the advantage over doing something like:

const nextTodo = await loadTodoFromServer();
this.setState(update(this.state, { todos: { '$set', nextTodo } }));

@linq2js
Copy link

linq2js commented Aug 15, 2018

Im implementing base class for covering update state using immhelper, something like

class BaseComponent {
  async update(specs) {
    const nextState = await update(state, specs);
    if(nextState !== state) { this.setState({ ...nextState }); }
  }  
}

// it easy to use rather than this.setState(update(...))

and action logic is clearly:
loadTodo() {
   this.update({ todos: Api.loadTodos() })
}

@hoytech
Copy link

hoytech commented Aug 16, 2018

Yes it's an interesting concept, although I'm wondering if there could be a way to implement it as a wrapper around update() instead of modifying its internals.

@linq2js
Copy link

linq2js commented Aug 16, 2018

Look at this https://codesandbox.io/s/0p22jvvoow
This is my new package, it supports 5 ways to implement app

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

No branches or pull requests

5 participants