Optimise & Refactor Records #214

ronag · 2016-08-18T10:08:06Z

I know that currently this is not something that will get into deepstream but I would at least ask you to consider it, and possibly give some feedback, since we need it.

Our application is very record heavy and we have some problems with keeping stable 60 fps. We've already improved on this issue with #200 but we still have some performance issues.

A part form general refactoring and simplification:

Optimizations:

Optional deep copies. Assume immutability.
Cache path tokenisation.
Reduced allocations.
Reduced copies.

Fixes #172, #213

We could make even more use of Object.create( null ) over {} for jsonPath. However, jasmine test for types in isEqual which would make some tests fail.

coveralls · 2016-08-18T10:16:07Z

Coverage decreased (-40.05%) to 57.194% when pulling c22b58d on nxtedition:feature/optimize-records into d6af1cc on deepstreamIO:master.

coveralls · 2016-08-18T10:39:32Z

Coverage decreased (-40.05%) to 57.194% when pulling a15181a on nxtedition:feature/optimize-records into d6af1cc on deepstreamIO:master.

coveralls · 2016-08-18T10:40:56Z

Coverage decreased (-40.05%) to 57.194% when pulling ad35058 on nxtedition:feature/optimize-records into d6af1cc on deepstreamIO:master.

coveralls · 2016-08-18T12:40:21Z

Coverage decreased (-40.05%) to 57.197% when pulling f141a79 on nxtedition:feature/optimize-records into d6af1cc on deepstreamIO:master.

yasserf · 2016-08-18T12:40:44Z

Hey Ronag,

Interesting PR. I like the refactor for the path, I done the logic similar for the java client.

Just had an interesting conversation about object.create( null ) which means we will officially drop IE8 support ( unless an external polyfill is provided ). That's fine though so its good.

Regarding deepCopy, we have alot of less experienced developers assume that a copy is provided back. If you allow deepCopy ( default true ) to be an option you pass into deepstreamClient on load that will be acceptable, but we can't change it without it.

No deep equals. Assume reference equality and that set is only called when actually changed.

That's interesting, since yes it makes it faster but it also means listeners would get notified with same data constantly if your setting equal data. Adding another flag would also be beneficial, ie deepCompare

If you could also give some before and after metrics to updates that would interesting.

Cheers!

ronag · 2016-08-18T14:08:24Z

That's interesting, since yes it makes it faster but it also means listeners would get notified with same data constantly if your setting equal data.

In our case we have to perform that check anyway upstream in our application. So we basically end up with a lot of unnecessary checks.

if your setting equal data

Which should be the exception rather than the rule.

coveralls · 2016-08-18T14:12:56Z

Coverage decreased (-40.03%) to 57.217% when pulling e0a8d41 on nxtedition:feature/optimize-records into d6af1cc on deepstreamIO:master.

ronag · 2016-08-18T14:16:30Z

@yasserf: Take a look now. I've made the behaviour configurable.

coveralls · 2016-08-18T14:19:01Z

Coverage increased (+0.002%) to 97.249% when pulling 870affd on nxtedition:feature/optimize-records into d6af1cc on deepstreamIO:master.

coveralls · 2016-08-18T14:29:30Z

Coverage decreased (-0.2%) to 97.089% when pulling 07d8752 on nxtedition:feature/optimize-records into d6af1cc on deepstreamIO:master.

coveralls · 2016-08-18T14:31:13Z

Coverage decreased (-0.2%) to 97.089% when pulling 1b70a36 on nxtedition:feature/optimize-records into d6af1cc on deepstreamIO:master.

coveralls · 2016-08-18T14:33:17Z

Coverage decreased (-0.2%) to 97.089% when pulling 1b70a36 on nxtedition:feature/optimize-records into d6af1cc on deepstreamIO:master.

coveralls · 2016-08-18T22:22:55Z

Coverage increased (+0.02%) to 97.268% when pulling 1321c64 on nxtedition:feature/optimize-records into d6af1cc on deepstreamIO:master.

coveralls · 2016-08-18T22:59:01Z

Coverage decreased (-77.6%) to 19.644% when pulling 3fb098c on nxtedition:feature/optimize-records into d6af1cc on deepstreamIO:master.

coveralls · 2016-08-18T22:59:51Z

Coverage decreased (-77.6%) to 19.644% when pulling 3fb098c on nxtedition:feature/optimize-records into d6af1cc on deepstreamIO:master.

coveralls · 2016-08-18T23:03:35Z

Coverage decreased (-65.7%) to 31.572% when pulling 64a4627 on nxtedition:feature/optimize-records into d6af1cc on deepstreamIO:master.

coveralls · 2016-08-18T23:04:24Z

Coverage decreased (-65.4%) to 31.852% when pulling 5bd4fe0 on nxtedition:feature/optimize-records into d6af1cc on deepstreamIO:master.

coveralls · 2016-08-18T23:05:26Z

Coverage decreased (-65.4%) to 31.852% when pulling 5bd4fe0 on nxtedition:feature/optimize-records into d6af1cc on deepstreamIO:master.

coveralls · 2016-08-18T23:08:51Z

Coverage decreased (-65.4%) to 31.852% when pulling 9f15a46 on nxtedition:feature/optimize-records into d6af1cc on deepstreamIO:master.

coveralls · 2016-08-18T23:09:08Z

Coverage decreased (-65.4%) to 31.852% when pulling 9f15a46 on nxtedition:feature/optimize-records into d6af1cc on deepstreamIO:master.

coveralls · 2016-08-18T23:11:01Z

Coverage decreased (-65.4%) to 31.852% when pulling 2fc9943 on nxtedition:feature/optimize-records into d6af1cc on deepstreamIO:master.

coveralls · 2016-08-18T23:12:16Z

Coverage decreased (-65.9%) to 31.338% when pulling 2fc9943 on nxtedition:feature/optimize-records into d6af1cc on deepstreamIO:master.

ronag · 2016-08-18T23:13:18Z

This ended up being a major refactoring & optimisation.

…ther with the other async races.

ronag · 2016-08-26T09:11:00Z

Not sure why this is failing all of a sudden. All test pass fine on my local copy.

Critical error occured on deepstream PLUGIN_ERROR Error from messageConnector plugin: REDIS error:Received message for unknown topic CL

ronag · 2016-08-26T12:32:13Z

Note that this still has the "bad" conflict resolve behaviour (see #224) from the previous implementation. Otherwise all test don't pass.

ronag · 2016-08-26T12:39:12Z

src/record/record.js

+		}
+
+		this._sendUpdate( undefined, data );
+		this._applyChange( newValue );


To fix #224 this should be:

if ( !utils.deepEquals( data, remoteData ) ) { this._sendUpdate( undefined, data ); } this._applyChange( jsonPath.set( this._$data, undefined, data, false ) );

Makes sense!

yasserf · 2016-09-01T21:26:42Z

I'm very keen on getting this PR merged and used as a baseline for the other async cases you mentioned. We have been doing some performance tests on deepstream and listening this entire week to ensure things work under heavy load and because of that we haven't been able to look into it yet.

I will most likely merge this in once I guarantee tests are passing, and release it under a 1.1RC along side listening while we continue doing stress testing over the next couple of weeks. I apologise for the time mixup / delay, given the amount of new features in 1.1 we want to thoroughly test it with all our internal use cases before we release it into the wild!

ronag · 2016-09-01T22:31:21Z

src/record/record.js

+		this.whenReady( function () {
+			this._eventEmitter.on( args.path, args.callback );
+			args.callback( this.get( args.path ) );
+		}.bind(this) );


In order to resolve #229 I think this should be:

this._eventEmitter.on( args.path, args.callback ); if ( this.isReady ) { args.callback( this.get( args.path ) ); }

Of course this also requires that the initial read invokes callbacks, i.e. initial {} !== read {}

yasserf · 2016-09-04T14:20:05Z

src/record/json-path.js

+function patch( oldValue, newValue, deepCopy ) {
+	var i;
+
+	if ( utils.deepEquals( oldValue, newValue ) ) {


I'm a little confused, won't this run recursively every time and outweigh the benefit of having the deepCopy flag?

I can see it being an advantage when the flag is false since it will do literal object comparison, but when the flag is true it will have to do this on every object..

return deepCopy !== false ? utils.deepCopy( newValue ) : newValue;

@yasserf: Not sure what you are worried about? The extra comparison? I would expect the performance impact to be negligible due to branch prediction. I would even expect V8 to optimize it when it notices that the parameter is effectively constant.

The extra comparisonS.

deepEqual does a stringify twice. Since this is a recursive function it will do so on each individual item within the path.

If you compare the disadvantage of that with anyone who uses the deepCopy as default: true
to the advantage of when it is false I'm wondering how much more of a hit we are adding to the default usecase as a result.

The point here is also to maintain reference equality (even if deepCopy is true). patch should maintain references if the new value is equal. This is so we don't need to use deepEquals in _applyChange. See, #213.

Essentially, this also allows that any (including _applyChange) e.g. React component that is backed by a deepstream record can use shallowCompare.

@yasserf: You mean the overlap of deepEquals. Yes, that is a future improvement I documented somewhere.

The point here is also to maintain reference equality (even if deepCopy is true). patch should maintain references if the new value is equal.

Gotcha.

@yasserf: It would only be a problem in practice for large deeply nested records...

yasserf · 2016-09-04T14:54:41Z

Done a rebase onto master and added missing documentation.

Merging it via #239

yasserf added the awaiting review label Aug 18, 2016

ronag force-pushed the feature/optimize-records branch from 64a4627 to 5bd4fe0 Compare August 18, 2016 23:04

ronag force-pushed the feature/optimize-records branch 2 times, most recently from 28f38c8 to 9f15a46 Compare August 18, 2016 23:07

ronag force-pushed the feature/optimize-records branch 2 times, most recently from d3e959b to 2fc9943 Compare August 18, 2016 23:10

ronag force-pushed the feature/optimize-records branch from 2fc9943 to d394fb4 Compare August 18, 2016 23:12

ronag added 2 commits August 21, 2016 21:09

json-path: Removed legacy ASTERIX.

072f9c2

Cosmetics

9906b88

ronag mentioned this pull request Aug 21, 2016

Check for invalid paths #219

Closed

ronag added 5 commits August 21, 2016 22:33

json-path: Use regex match for future path validation.

99616a4

Cosmetics.

e416c99

json-path: Check validity of data + path on get.

79e9d07

Cosmetics

cc781ef

Cosmetics

6e34fec

ronag force-pushed the feature/optimize-records branch from 216e644 to ee22eb8 Compare August 22, 2016 08:26

deepEquals: Short-circuit on reference equality.

0fd3844

ronag force-pushed the feature/optimize-records branch from ee22eb8 to 0fd3844 Compare August 22, 2016 08:27

ronag mentioned this pull request Aug 22, 2016

Record/patch #220

Closed

Cancel resolve if destroyed. This needs to be fixed better later toge…

37c08b1

…ther with the other async races.

ronag mentioned this pull request Aug 24, 2016

merge with local wins is never sent to server #224

Closed

ronag added 2 commits August 26, 2016 11:02

patch target to allow reference equality

fe0f5de

check isDestroyed in _applyChange.

683a331

ronag reviewed Aug 26, 2016
View reviewed changes

ronag mentioned this pull request Aug 31, 2016

Fix record discard/delete & subscribe race #209

Closed

ronag reviewed Sep 1, 2016
View reviewed changes

Handle path==='undefined'

f2d0cc3

ronag mentioned this pull request Sep 2, 2016

Fix/#229 #234

Closed

ronag added a commit to nxtedition/deepstream.io-client-js that referenced this pull request Sep 2, 2016

deepstreamIO#214

41f7fa9

yasserf reviewed Sep 4, 2016
View reviewed changes

yasserf closed this Sep 4, 2016

yasserf removed the awaiting review label Sep 4, 2016

ronag deleted the feature/optimize-records branch November 14, 2018 18:58

Optimise & Refactor Records #214

Optimise & Refactor Records #214

Conversation

ronag commented Aug 18, 2016 • edited Loading

coveralls commented Aug 18, 2016

coveralls commented Aug 18, 2016

coveralls commented Aug 18, 2016

coveralls commented Aug 18, 2016

yasserf commented Aug 18, 2016

ronag commented Aug 18, 2016 • edited Loading

coveralls commented Aug 18, 2016

ronag commented Aug 18, 2016

coveralls commented Aug 18, 2016

coveralls commented Aug 18, 2016

coveralls commented Aug 18, 2016

coveralls commented Aug 18, 2016

coveralls commented Aug 18, 2016

coveralls commented Aug 18, 2016

coveralls commented Aug 18, 2016

coveralls commented Aug 18, 2016

coveralls commented Aug 18, 2016

coveralls commented Aug 18, 2016

coveralls commented Aug 18, 2016

coveralls commented Aug 18, 2016

coveralls commented Aug 18, 2016

coveralls commented Aug 18, 2016

ronag commented Aug 18, 2016

ronag commented Aug 26, 2016

ronag commented Aug 26, 2016 • edited Loading

ronag Aug 26, 2016 • edited Loading

Choose a reason for hiding this comment

yasserf Sep 4, 2016

Choose a reason for hiding this comment

yasserf commented Sep 1, 2016

ronag Sep 1, 2016 • edited Loading

Choose a reason for hiding this comment

yasserf Sep 4, 2016

Choose a reason for hiding this comment

ronag Sep 4, 2016 • edited Loading

Choose a reason for hiding this comment

yasserf Sep 4, 2016

Choose a reason for hiding this comment

ronag Sep 4, 2016 • edited Loading

Choose a reason for hiding this comment

ronag Sep 4, 2016 • edited Loading

Choose a reason for hiding this comment

ronag Sep 4, 2016

Choose a reason for hiding this comment

yasserf Sep 4, 2016

Choose a reason for hiding this comment

ronag Sep 4, 2016 • edited Loading

Choose a reason for hiding this comment

yasserf commented Sep 4, 2016

ronag commented Aug 18, 2016 •

edited

Loading

ronag commented Aug 18, 2016 •

edited

Loading

ronag commented Aug 26, 2016 •

edited

Loading

ronag Aug 26, 2016 •

edited

Loading

ronag Sep 1, 2016 •

edited

Loading

ronag Sep 4, 2016 •

edited

Loading

ronag Sep 4, 2016 •

edited

Loading

ronag Sep 4, 2016 •

edited

Loading

ronag Sep 4, 2016 •

edited

Loading