Fix #43 - Search page UI responsiveness sluggish when many spans in results #46

tiffon · 2017-07-31T18:55:48Z

Fix an issue with the search page that shows up when there are many spans (cumulatively) in the search results. Issue at hand is the mapStateToProps function calls .toJS() on some of the immutableJS state values, transforms the resultant values, then passes the transformed values to reselect. The .toJS() invocation prevents the calculations from being memoized by reselect because it always results in new objects or arrays.

The change is to wrap the .toJS() call and subsequent transformation in a function, the result of which is cached based on the state value. The caching resolves the issue. Only the last result needs to be cached because these are top-level state values (vs transforming items in an array) and therefore a state value will never be revisited.

Fix an issue with the search page that shows up when there are many spans (cumulatively) in the search results. Issue at hand is the mapStateToProps function calls .toJS() on some of the immutableJS state values, transforms the resulting values, then passes the transformed values to reselect. The .toJS() invocation prevents the calculations from being memoized by reselect because it always results in new objects or arrays. The change is to wrap the .toJS() call and subsequent transformation in a function, the result of which is cached based on the state value. The caching resolves the issue. Only the last result needs to be cached because these are top-level state values (vs transforming items in an array) and therefore a state value will never be revisited.

yurishkuro · 2017-07-31T19:12:23Z

src/utils/get-last-xform-cacher.js

+ *
+ * @param  {function} xformer The transformer function, the most recent result
+ *                            of which is cached.
+ * @return {function} A wrapper around the transformer function which which


which which

yurishkuro · 2017-07-31T19:14:49Z

src/components/SearchTracePage/index.js

@@ -218,20 +220,32 @@ SearchTracePage.propTypes = {
  errorMessage: PropTypes.string,
 };

+const stateTraceXformer = getLastXformCacher(stateTrace => {
+  const { traces: traceMap, loading, error: traceError } = stateTrace.toJS();


Q (curious): how is stateTrace represented in the first place? What does the call to toJS() achieve?

The state is an Immutable.js Map. .toJS() converts it to a plain JS object. The .toJS() call is probably superfluous; I'll go over why I didn't take it out in our discussion Wednesday.

yurishkuro

lgtm

saminzadeh · 2017-07-31T19:24:04Z

src/utils/get-last-xform-cacher.js

+ *                    caches the last transformation, returning the cached
+ *                    value if the source value is the same.
+ */
+export default function getLastXformCacher(xformer) {


Shouldn't reselect or lodash memoize do this?

@saminzadeh Most likely not as we only need the last calculated value to be cached rather than the function to be fully memoized.

coveralls · 2017-07-31T19:29:50Z

Coverage increased (+0.2%) to 55.153% when pulling 19697ff on tiffon:bug/043-search-page-responsiveness into c30737e on uber:master.

coveralls · 2017-07-31T19:30:01Z

Coverage increased (+0.2%) to 55.153% when pulling 19697ff on tiffon:bug/043-search-page-responsiveness into c30737e on uber:master.

saminzadeh · 2017-08-01T12:56:56Z

src/utils/get-last-xform-cacher.js

+  return function getOrCache(...args) {
+    const sameArgs = lastArgs &&
+      lastArgs.length === args.length &&
+      lastArgs.every((lastArg, i) => lastArg === args[i]);


If .toJS() is returning a new reference, would lastArg === args[i] fail the test?

Also, _.memoize does the same thing but stores any existing computations rather than just the last computation. You can also pass a custom cache resolver if we need to
https://lodash.com/docs/4.17.4#memoize

@saminzadeh The immutable JS values are the params to the transform functions; .toJS() is called within the transform functions, so the caching with the param as the key should be fine.

In this case, we only care about caching the last calculation. Params to the transform function will never be revisited, so we don't need to cache anything other than the most recent calculation.

The reason the params will never be revisited is because they are top level values on the state, traces and services. When the search results change, traces will be different, and the previous value will never be revisited even if an identical search is executed. Same thing with services.

…esults (#46) * update eslint-plugin-react, avoid eslintrc warnings * .eslintrc env for browser, jest and jasmine * Fix #43 - Search page responsiveness issues Fix an issue with the search page that shows up when there are many spans (cumulatively) in the search results. Issue at hand is the mapStateToProps function calls .toJS() on some of the immutableJS state values, transforms the resulting values, then passes the transformed values to reselect. The .toJS() invocation prevents the calculations from being memoized by reselect because it always results in new objects or arrays. The change is to wrap the .toJS() call and subsequent transformation in a function, the result of which is cached based on the state value. The caching resolves the issue. Only the last result needs to be cached because these are top-level state values (vs transforming items in an array) and therefore a state value will never be revisited. * Fix yarn lock file, e.g. use npm registry * Fix typo in comment Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>

tiffon added 3 commits July 31, 2017 12:30

update eslint-plugin-react, avoid eslintrc warnings

54ed481

.eslintrc env for browser, jest and jasmine

34ea513

yurishkuro reviewed Jul 31, 2017

View reviewed changes

yurishkuro approved these changes Jul 31, 2017

View reviewed changes

tiffon added 2 commits July 31, 2017 15:20

Fix yarn lock file, e.g. use npm registry

9aef93d

Fix typo in comment

19697ff

saminzadeh reviewed Jul 31, 2017

View reviewed changes

saminzadeh reviewed Aug 1, 2017

View reviewed changes

tiffon merged commit 6979e20 into jaegertracing:master Aug 1, 2017

tiffon mentioned this pull request Sep 14, 2017

slow UI / hangs browser jaegertracing/jaeger#386

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #43 - Search page UI responsiveness sluggish when many spans in results #46

Fix #43 - Search page UI responsiveness sluggish when many spans in results #46

tiffon commented Jul 31, 2017

yurishkuro Jul 31, 2017

tiffon Jul 31, 2017

yurishkuro Jul 31, 2017

tiffon Jul 31, 2017

yurishkuro left a comment

saminzadeh Jul 31, 2017

tiffon Jul 31, 2017

coveralls commented Jul 31, 2017

coveralls commented Jul 31, 2017

saminzadeh Aug 1, 2017

tiffon Aug 1, 2017

Fix #43 - Search page UI responsiveness sluggish when many spans in results #46

Fix #43 - Search page UI responsiveness sluggish when many spans in results #46

Conversation

tiffon commented Jul 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 31, 2017

coveralls commented Jul 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment