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.

Sign up for GitHub

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

Jump to bottom

perf(classnames): use arguments instead spread #467

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

perf(classnames): use arguments instead spread #467

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0892a84
perf(classnames): use arguments instead spread
yarastqt Aug 14, 2019
a5a644e
style(classnames): wrap to block if statement
yarastqt Aug 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions packages/classnames/classnames.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,24 @@
*
* @param strings classNames strings
*/
export function classnames(...strings: Array<string | undefined>) {
export function classnames(...strings: Array<string | undefined>): string;
export function classnames() {
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);
Copy link

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 •
edited
Loading

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 •
edited
Loading

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

Copy link

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 •
edited
Loading

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)

Copy link

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

Copy link

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


for (const value of classNameList) {
if (value === '' || uniqueCache.has(value)) {
if (value === '' || value === undefined || uniqueCache.has(value)) {
continue;
}

uniqueCache.add(value);
if (className.length > 0) className += ' ';

if (className.length > 0) {
className += ' ';
}

className += value;
}

Expand Down