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

fix(table): use stable sort algorithm to prevent SSR issues #1399

Merged
merged 4 commits into from Nov 24, 2017

Conversation

Projects
None yet
3 participants
@tmorehouse
Member

tmorehouse commented Nov 23, 2017

Different JavaScript environments implement native array sorting differently, and in some cases are implemented with an unstable/inconsistent routine (Chrome and V8 are examples where native sort is inconsistent/unstable).

This can produce SSR client mismatch errors in different environments when sortBy is preset on initial server render of <b-table>, where the server native sort uses one method while the client native sort uses a different algorithm.

This PR introduces a new utility function stableSort, of which <b-table> will use instead of native sort.

stableSort uses the native sort, and will fallback to sorting by index when the entries compare equal. This introduces a performance penalty over native sort, but prevents SSR DOM mismatch errors.

tmorehouse added some commits Nov 23, 2017

@tmorehouse tmorehouse requested review from pi0, alexsasharegan and mosinve Nov 23, 2017

@tmorehouse tmorehouse changed the title from fix(table): use stable sort algoritm to fix(table): use stable sort algorithm Nov 23, 2017

@tmorehouse tmorehouse changed the title from fix(table): use stable sort algorithm to fix(table): use stable sort algorithm to prevent SSR issues Nov 23, 2017

@tmorehouse tmorehouse removed the Type: Bug label Nov 23, 2017

export default function stableSort (array, compareFn) {
return array
.map((a, index) => [ index, a ])
.sort(function (a, b) { return this(a[1], b[1]) || a[0] - b[0] }.bind(compareFn))

This comment has been minimized.

@mosinve

mosinve Nov 24, 2017

Member

This works the same way:
.sort( (a, b) => compareFn(a[1], b[1]) || (a[0] - b[0]) )

But it's more declarative/readable way

This comment has been minimized.

@tmorehouse

tmorehouse Nov 24, 2017

Member

Binding the compareFn as the this context speeds up performance about 35%

This comment has been minimized.

@mosinve

mosinve Nov 24, 2017

Member

did you test it? o_O

This comment has been minimized.

@mosinve

mosinve Nov 24, 2017

Member

.sort(function (a, b) { return this(a[1], b[1]) || a[0] - b[0] }.bind(compareFn))
bind

.sort((a, b) => compareFn(a[1], b[1]) || (a[0] - b[0]))
arrow

This comment has been minimized.

@tmorehouse

tmorehouse Nov 24, 2017

Member

Based on the original source: https://stackoverflow.com/questions/1427608/fast-stable-sorting-algorithm-implementation-in-javascript/45422645#45422645

Using .bind(compareFunction) on the wrapped anonymous function within stableSort() boosted that relative performance from about 38% by avoiding an unneeded scoped reference to compareFunction on each call by assigning it to the context instead.

With larger arrays, the improvement would be more noticeable

This comment has been minimized.

@tmorehouse

tmorehouse Nov 24, 2017

Member

But the babel transpile will not bind the compareFn function as this, babel (transpiling the arrow function) will set this to the this context of the location it was defined, which is the outer stableSort function context.

This code explicitly sets this as the compareFn, so saving the function call setup on each iteration of the inner loop.

Since this function will be called every time the table needs updating (when there is a sort change), speed makes the difference with large datasets, (except when localSort is disabled (and the user is pre-sorting the data before sending to b-table)

This comment has been minimized.

@tmorehouse

tmorehouse Nov 24, 2017

Member

Note that this in a method shorthand or function refers to the object literal itself whereas this in an arrow function refers to the scope outside the object literal.

In this case, we do not want this to refer to the stableSort context (the outer function), we want this to refer to the compareFn (so calling this(a, b) actually calls compareFn, without the overhead of calling compareFn each iteration fo the .sort(..) routine)

This comment has been minimized.

@tmorehouse

tmorehouse Nov 24, 2017

Member

I have added comment above the code to explain the reasoning.

This comment has been minimized.

@tmorehouse

tmorehouse Nov 24, 2017

Member

Note that a stable sort is much less efficient than using QuickSort (which is what V8 and Chrome use if the array length is > 10), so we are taking a larger performance hit by implementing stable sort in order to avoid SSR errors. So being able to squeeze some speed improvements is needed.

@codecov-io

This comment has been minimized.

codecov-io commented Nov 24, 2017

Codecov Report

Merging #1399 into dev will not change coverage.
The diff coverage is 50%.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #1399   +/-   ##
=======================================
  Coverage   42.46%   42.46%           
=======================================
  Files         130      130           
  Lines        2915     2915           
  Branches      901      902    +1     
=======================================
  Hits         1238     1238           
  Misses       1277     1277           
  Partials      400      400
Impacted Files Coverage Δ
src/components/table/table.vue 73.05% <50%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37ab5cb...4a60602. Read the comment docs.

@tmorehouse tmorehouse merged commit 552c438 into dev Nov 24, 2017

2 checks passed

License Compliance License checks passed.
Details
ci/circleci Your tests passed on CircleCI!
Details

@tmorehouse tmorehouse deleted the table/stable-sort branch Nov 24, 2017

pi0 added a commit that referenced this pull request Nov 29, 2017

fix(table): use stable sort algorithm to prevent SSR issues (#1399)
* Create stable-sort.js

* add stable sort to utils

* [b-table] use stableSort
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment