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

perf(classnames): use arguments instead spread #467

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yarastqt
Copy link
Member

No description provided.

let className = '';
const uniqueCache = new Set();
const classNameList = strings.join(' ').split(' ');
const classNameList: string[] = [].slice.call(arguments);
Copy link
Member Author

Choose a reason for hiding this comment

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

Нужно поправить тип Array<string | undefined>

@yarastqt
Copy link
Member Author

cc @victor-homyakov

let className = '';
const uniqueCache = new Set();
const classNameList = strings.join(' ').split(' ');
// Use arguments instead rest operator for better performance.
const classNameList: Array<string | undefined> = [].slice.call(arguments);

Choose a reason for hiding this comment

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

  • ломается поведение при вызове classnames('a b') (да, оно сейчас не зафиксировано в unit-тестах, но strings.join(' ').split(' ') было сделано именно для такого кейса)
  • в старых версиях V8 [].slice.call(arguments) медленнее цикла for, в который транслируется ...strings (функция с передачей arguments за пределы scope не оптимизировалась https://github.com/petkaantonov/bluebird/wiki/optimization-killers#32-leaking-arguments)

Copy link

@victor-homyakov victor-homyakov Aug 15, 2019

Choose a reason for hiding this comment

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

Да и сейчас for пока что как минимум не хуже остальных способов, а slice наоборот один из самых медленных вариантов

http://jsbench.github.io/#1e36f485d47f9cf8009eec50a1b80457 как один из примеров бенчмарка способов конверсии arguments в массив

Copy link
Member Author

Choose a reason for hiding this comment

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

В итоге оставляем пока рест оператор?

Copy link
Member

@belozer belozer Aug 15, 2019

Choose a reason for hiding this comment

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

А давайте вместо slice сделаем просто 2 for?
http://jsbench.github.io/#0eac5d083e6b6e05a1614f26feefd1df

Спойлер:
PR (slice)
227,190 ops/sec

Предложение (2 x for, включая кейс classnames('a b'))
373,978 ops/sec

Актуальный код (rest)
133,817 ops/sec

Choose a reason for hiding this comment

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

В бенчмарке http://jsbench.github.io/#0eac5d083e6b6e05a1614f26feefd1df баг в строке for (let i; i < arguments.length; i++) {

Copy link
Member Author

Choose a reason for hiding this comment

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

У нас не будет for of в любом случае, пока мы поддерживаем ie11, сейчас for of компилируется в обычный for(;;)

Copy link
Member

@belozer belozer Aug 19, 2019

Choose a reason for hiding this comment

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

@victor-homyakov примеры со slice заранее не честные, т.к. у них пропущен кейс classnames('a b'), т.е. это не 2 класса, а один. Если в примеры со slice добавить split, то его производительность значительно упадёт.

Вот честное сравнение slice/for http://jsbench.github.io/#35ec4a8199d42ffc7de83998f0de34f8
(странно что в Firefox кейс со slice быстрее, чем for)

Choose a reason for hiding this comment

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

По ссылке http://jsbench.github.io/#35ec4a8199d42ffc7de83998f0de34f8 не вижу правильного варианта slice

Choose a reason for hiding this comment

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

https://jsperf.com/bem-react-classnames вот так вижу, что вариант for + for лучше выглядит во всех проверенных браузерах (Chrome, Safari, Firefox)

Copy link
Member

Choose a reason for hiding this comment

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

Давайте тогда на for + for остановимся. Лучше решения не вижу на данный момент. @yarastqt

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.

4 participants