dai-shi / proxy-memoize

Intuitive magical memoization library with Proxy and WeakMap
http://proxy-memoize.js.org/
MIT License
715 stars 16 forks source link

Returns object which not equal by ref #63

Closed Bardiamist closed 1 year ago

Bardiamist commented 1 year ago

I want to optimize my calculations with simplest memoization using ref but faced with problem that memoized state props not equal to previous by ref. Why? Can I compare these values by ref?

const memoize = require('proxy-memoize').memoize;

const reduxState = {
  bookByName: {},
};

const selector = memoize((state) => {
  if (state.bookByName !== reduxState.bookByName) {
    console.log('Why not equal?');
  }
});

selector(reduxState);
dai-shi commented 1 year ago

Because the state is wrapped by a proxy. That's how proxy-memoize works. Can't you do the optimization outside memoize??

Bardiamist commented 1 year ago

I want to create simple selector which will check that one prop changed

Something like this but more expensive:

const memoize = require('proxy-memoize').memoize;

const reduxState = {
  bookByName: {},
};

let prevBookByName;
let prevBooks = [];

const selectBooks = (state) => {
  if (state.bookByName !== prevBookByName) {
    prevBookByName = state.bookByName;
    prevBooks = Object.values(prevBookByName);
  }

  return prevBooks;
};

const selectActiveBooks = memoize((state) => selectBooks(state).map((book) => book.isActive));

It will work 100 times faster than if wrap selectBooks into memoize

So no way to compare these objects or something?

dai-shi commented 1 year ago

That seems like an unnecessary optimization with proxy-memoize.

Is this slow?

const selectActiveBooks = memoize((state) => Object.values(state.bookByName).map((book) => book.isActive));
Bardiamist commented 1 year ago

In my real project the selectActiveBooks selector makes more actions. And takes 0.1 ms (if wrap into memoize and get memoized value) on my machine when optimized selector takes 0.001 ms. Also this selector will call often and will take more time on mobile.

dai-shi commented 1 year ago

I'm not sure if I understand the structure, but how about this? Does it work in the real project?

const selectBooks = memozie((state) => Object.values(state.bookByName));

const selectActiveBooks = memoize((state) => selectBooks(state).map((book) => book.isActive));
Bardiamist commented 1 year ago

I'm not sure if I understand the structure, but how about this? Does it work in the real project?

const selectBooks = memozie((state) => Object.values(state.bookByName));

const selectActiveBooks = memoize((state) => selectBooks(state).map((book) => book.isActive));

In this case selectBooks takes 0.1 ms

When this:

let prevBookByName;
let prevBooks = [];

const selectBooks = (state) => {
  if (state.bookByName !== prevBookByName) {
    prevBookByName = state.bookByName;
    prevBooks = Object.values(prevBookByName);
  }

  return prevBooks;
};

very fast (takes 0.001 ms)

dai-shi commented 1 year ago

Hm, that sounds like too much for the overhead of proxies. Essentially, two styles are identical, if I don't misunderstand.

Can you reproduce the benchmark with https://jsbench.me/ ?

dai-shi commented 1 year ago

I went ahead and tried it. https://jsbench.me/pgldcdcqag/1

image

proxy-memoize is 66% slower.

Bardiamist commented 1 year ago

In real project selectBooks use filter and sort where call another selectors for get and compire BigNumbers (bignumber.js).

So I just need ability to comprire objects by ref. I added example for it on first post. Maybe we can have something like: state.bookByName.getUntracked() !== reduxState.bookByName?

Otherwise I will just store books in Redux instead of select.

Bardiamist commented 1 year ago

Yes, looks it works

const selectBooks = (state) => {
  const untrackedBookByName = getUntracked(state.bookByName);

  if (untrackedBookByName != null && untrackedBookByName !== prevBookByName) {
    prevBooks = Object.values(state.bookByName);
    prevBookByName = untrackedBookByName;
  }

  return prevBooks;
};

But why documentation says:

[Notes] This function is for debugging purpose. It's not supposed to be used in production and it's subject to change.

Can I use it in prod?

Update: I see it broke another selectors. Looks when we call getUntracked it's not just returns untracked object but also prevent all next tracking? Because looks it's not triggers on Object.values(state.bookByName)

Bardiamist commented 1 year ago

In summary I faced with 2 or 3 problems on 2.0.1 version so reverted back to 1.0.0 One of problems that memoized selector don't calls when inner memoized selector returns changed value. Maybe because it returns BigNumber. Maybe will create repro later.

Bardiamist commented 1 year ago

If add setectBookByName as here then everything should work as expected, right?

const memoize = require('proxy-memoize').memoize;

const reduxState = {
  bookByName: {},
};

let prevBookByName;
let prevBooks = [];

const setectBookByName = memoize((state) => state.bookByName);

const selectBooks = (state) => {
  const bookByName = setectBookByName(state);

  if (bookByName !== prevBookByName) {
    prevBookByName = bookByName;
    prevBooks = Object.values(bookByName);
  }

  return prevBooks;
};

const selectActiveBooks = memoize((state) => selectBooks(state).map((book) => book.isActive));

In proxy-memoize@1.0.0 it takes 0.005

When const selectBooks = memozie((state) => Object.values(state.bookByName)); (with difficult filter and sort also) takes 2ms

dai-shi commented 1 year ago

I'm a little bit lost. For performance issue that differs in v1.0.0 and v2.0.1, could you create a minimal reproduction so that I can create a test case and investigate it?

For the hand optimization, I'm still not sure where's the bottleneck.

Bardiamist commented 1 year ago

I'll try to create minimal repro

My today's example works fast and should not lead to some unexpected behaviour, right?

dai-shi commented 1 year ago

Not 100% sure, but I don't think it changes. Wouldn't it be still a proxy?

Bardiamist commented 1 year ago

Ok, It was very dificult to find and reproduce the same order of related selectors but I got it

Minimal repro ``` const BigNumber = require('bignumber.js'); const memoize = require('proxy-memoize').memoize; const reduxState = { "strangeObject": {}, "books": { "bookByName": { "book1": { "priceString": "150", "info": { "name": "book1" } }, "book2": { "priceString": "50", "info": { "name": "book2" } } } } }; const zeroBigNumber = new BigNumber(0); let prevSortedBooks = []; let prevBookByName = {}; const createSelectorById = (getById) => { let memoizedSelectorById = {}; return (id) => { const memoizedSelector = memoizedSelectorById[id]; if (memoizedSelector != null) { return memoizedSelector; } const nextMemoizedSelector = memoize(getById(id)); memoizedSelectorById[id] = nextMemoizedSelector; return nextMemoizedSelector; }; }; const getPriceBigNumberSelector = createSelectorById(( infoName, ) => ({ books: { bookByName, }, }) => { const book = bookByName[infoName]; if (book != null) { return new BigNumber(book.priceString); } return zeroBigNumber; }); const getIsExpensiveSelector = createSelectorById(( infoName, ) => (state) => { const priceBigNumber = getPriceBigNumberSelector(infoName)(state); return priceBigNumber.isGreaterThanOrEqualTo(10); }); const selectBookByName = memoize(({ books: { bookByName, }, }) => bookByName); const selectSortedBooks = (state) => { const bookByName = selectBookByName(state); if (bookByName !== prevBookByName) { prevSortedBooks = Object.values(bookByName).sort(( { info: firstInfo, }, { info: secondInfo, }, ) => { const firstRoundPriceBigNumber = getRoundPriceBigNumberSelector( firstInfo.name, )(state); const secondRoundPriceBigNumber = getRoundPriceBigNumberSelector( secondInfo.name, )(state); if (firstRoundPriceBigNumber.isLessThan(secondRoundPriceBigNumber)) { return 1; } if (firstRoundPriceBigNumber.isGreaterThan(secondRoundPriceBigNumber)) { return -1; } return 0; }); prevBookByName = bookByName; } return prevSortedBooks; }; const getRoundPriceBigNumberSelector = createSelectorById(( infoName, ) => (state) => { const { strangeObject, } = state; return getPriceBigNumberSelector(infoName)(state); }); const selectPricesSumBigNumber = memoize((state) => { let pricesSum = zeroBigNumber; const sortedBalances = selectSortedBooks(state); sortedBalances.forEach(({ info, }) => { const roundPriceBigNumber = getRoundPriceBigNumberSelector( info.name, )(state); pricesSum = pricesSum.plus(roundPriceBigNumber); }); return pricesSum; }); const selectFormattedPricesSum = memoize((state) => { const pricesSumBigNumber = selectPricesSumBigNumber( state, ); return pricesSumBigNumber.toString(); }); const selectIsExpensiveBookFound = memoize(( state, ) => selectSortedBooks(state).some(({ info, }) => getIsExpensiveSelector(info.name)(state))); selectIsExpensiveBookFound(reduxState); const result1 = selectFormattedPricesSum(reduxState); const nextState = { ...reduxState, books: { ...reduxState.books, bookByName: { ...reduxState.books.bookByName, book2: { ...reduxState.books.bookByName.book2, priceString: '90', }, }, }, }; const result2 = selectFormattedPricesSum(nextState); console.log(result1, result2); ```

@dai-shi Plese look into this and fix. I'll back to it in Monday

Bardiamist commented 1 year ago

I didn't try to remove BigNumbers (bignumber.js). But I use it and it's good time to ask: previously I stored BigNumbers in redux but now I creating selectors for BigNumbers. Is it right way?

dai-shi commented 1 year ago

I'll have a look within a week. I'm not sure if it's fixable yet.

Bardiamist commented 1 year ago

I forgot to say what in this example: when we call selectFormattedPricesSum with nextState result should be another but it returns memoized (wrong) value

dai-shi commented 1 year ago

If it's a wrong behavior rather then a performance issue, can you make the reproduction a little smaller?

Bardiamist commented 1 year ago

Yes, as I said, I faced with 2 or 3 problems and started from most critical, here is wrong behavior. Other about performance but I hope if you will fix this most critical bug then maybe other problems will disappear.

can you make the reproduction a little smaller?

I have reduced it very much. Also was very difficult to find the right calls queue for reproduce this bug. So if I'll remove a sotheing else then the bug will disappear. So it's already the minimal repro.

dai-shi commented 1 year ago

But it still looks too big and I'd need to make it small to make a test case anyway. I'll try that in a week, but thought you understood it better.

dai-shi commented 1 year ago

I reduced the code with removing selectSortedBooks caching. It should still reproduce the problem. Can you confirm if it's the correct reproduction? https://codesandbox.io/s/wandering-smoke-k6irl3?file=/src/App.js

(once confirmed, my next step is to remove bignumber.js.)

By the way, the way this example uses memoize seems over-complicated (= less efficient) for proxy-memoize usage (does it come from reselect mental model? just curious). We should fix the bug anyway, though.

Bardiamist commented 1 year ago

result1 is 200, result2 should be 240. We still see 200 so yea, bug still here.

By the way, the way this example uses memoize seems over-complicated (= less efficient) for proxy-memoize usage (does it come from reselect mental model?

I don't used reselect. My app works with cryptocurrency exhange. I decided to use proxy-memoize for: calculate user balances total in some currency. For it we need to find best chain of markets and convert (multiply) prices. Markets changes often. I expected that I can rely on proxy-memoize but faced with problems. v1.0.0 still looks good. Hope everything will fixed. Anyway thank you for this library.

dai-shi commented 1 year ago

so yea, bug still here.

Gotcha. Yes, I understand it should output 200, 240.

I don't used reselect.

Oh, my misunderstanding then. It looks to me there's more efficient way in proxy-memoize. Anyway, our plan is:

  1. fix wrong behavior or a bug if any
  2. fix performance issue
  3. possibly look for better practice
dai-shi commented 1 year ago

https://codesandbox.io/s/keen-heisenberg-4pg6u0?file=/src/App.js

This is so much for today. Next step would be to eliminate createSelectorById.

```js const { memoize } = require("proxy-memoize"); const reduxState = { strangeObject: {}, books: { book1: { price: 150, name: "book1" }, book2: { price: 50, name: "book2" } } }; const createSelectorById = (getById) => { let memoizedSelectorById = {}; return (id) => { const memoizedSelector = memoizedSelectorById[id]; if (memoizedSelector) { return memoizedSelector; } const nextMemoizedSelector = memoize(getById(id)); memoizedSelectorById[id] = nextMemoizedSelector; return nextMemoizedSelector; }; }; const getPriceSelector = createSelectorById((name) => ({ books }) => { const book = books[name]; return book.price; }); const getRoundPriceSelector = createSelectorById((name) => (state) => { const { strangeObject } = state; return getPriceSelector(name)(state); }); const selectPricesSum = memoize((state) => { return Object.values(state.books).reduce((acc, book) => { return acc + getRoundPriceSelector(book.name)(state); }, 0); }); getRoundPriceSelector("book1")(reduxState); getRoundPriceSelector("book2")(reduxState); getPriceSelector("book1")(reduxState); getPriceSelector("book2")(reduxState); const result1 = selectPricesSum(reduxState); const nextState = { ...reduxState, books: { ...reduxState.books, book2: { ...reduxState.books.book2, price: 90 } } }; const result2 = selectPricesSum(nextState); console.log(result1, result2); export default () => null; ```
dai-shi commented 1 year ago

Reduced further: https://codesandbox.io/s/frosty-sun-cssreh?file=/src/App.js

```js const { memoize } = require("proxy-memoize"); // const memoize = (fn) => fn; const reduxState = { strangeObject: {}, bookPrices: { book1: 150, book2: 50 } }; const selectBook2Price = memoize((state) => state.bookPrices.book2); const selectBook2RoundPrice = memoize((state) => { const { strangeObject } = state; return selectBook2Price(state); }); const selectPricesSum = memoize((state) => { return state.bookPrices.book1 + selectBook2RoundPrice(state); }); selectBook2RoundPrice(reduxState); const result1 = selectPricesSum(reduxState); const nextState = { ...reduxState, bookPrices: { ...reduxState.bookPrices, book2: 90 } }; const result2 = selectPricesSum(nextState); console.log(result1, result2); export default () => null; ```
dai-shi commented 1 year ago

I added a failing test in #64. It looks like the bug is in v2.0.1, but not in v2.0.0. So, #60 was a bad fix, as I implied: https://twitter.com/dai_shi/status/1613867634952654849

dai-shi commented 1 year ago

Check #64, and see if it resolves.

https://codesandbox.io/s/react-forked-0c8ddg this one looks good.

(edit) No, it wasn't fixed...

(edit2) Now, it should. This is complicated..

Bardiamist commented 1 year ago

I tried this: https://ci.codesandbox.io/status/dai-shi/proxy-memoize/pr/64/builds/341175

yarn add https://pkg.csb.dev/dai-shi/proxy-memoize/commit/cfbb7227/proxy-memoize

Ok, looks good! Thank you

Prefrormance looks good, I did'nt noticed freezes which was introduced in 1.1.0

Also I see that this my trick works as expected. So not have questions about performance now.

What you can say about BigNumbers. Is it right way to create getPriceBigNumberSelector as I did?

dai-shi commented 1 year ago

I don't know the details, but here's what I would do: https://codesandbox.io/s/react-forked-d2lu6i?file=/src/App.js

My general advice is don't overuse memoize. Less is more performant. Use it where there's heavy computation and/or we need stable reference.

proxy-memoize doesn't care if the return value is stable or not, it only cares the "usage".

Bardiamist commented 1 year ago

proxy-memoize doesn't care if the return value is stable or not, it only cares the "usage".

Sounds not encouraging, I expected that it tracks and triggers stably exactly when needed

Anyway, thank you!

dai-shi commented 1 year ago

That's why I asked if you have a mental model from reselect. The way it works is different and so is the usage.

We look for contributors who can work on docs/guides. Looking forward to your coming back for some improvements on docs.

Thanks!

dai-shi commented 1 year ago

Published: https://www.npmjs.com/package/proxy-memoize/v/2.0.2