Split ObjectState into utilities and storage implementations #164

andrewimm · 2016-01-11T20:37:59Z

ObjectState used to both store object data and the perform the functionality to mutate it.
Now, to facilitate different behavior between single-instance mode and unique-instance mode, ObjectState simply contains the mutation behavior, independent of how it's stored.
This PR introduces SingleInstanceState and UniqueInstanceState, storage layers that are directly accessed by ParseObject, and use ObjectState as a utility.

This change is chiefly to prevent the memory leaks reported in #111

Essentially, there are 4 pieces:

Isolation of ObjectState into utility functions that have their data sources passed in
Implementation of SingleInstanceState and UniqueInstanceState
Reimplementation of tests for both of these storage systems
Branching of ParseObject logic to interact with one or the other

I'll be adding more tests, but things look good for now.

andrewimm · 2016-01-11T20:39:01Z

Final code piece for #136

andrewimm · 2016-01-11T20:40:26Z

This change also brings ObjectState and the two State implementations to complete code coverage in unit tests.

andrewimm · 2016-01-11T22:39:20Z

Just tried allocating 10MM objects with UniqueInstanceState while profiling node, watched it continually GC and recover memory :D

roddylindsay · 2016-01-11T22:40:51Z

TylerBrock · 2016-01-11T22:52:43Z

w00t
On Mon, Jan 11, 2016 at 2:40 PM Roddy Lindsay [email protected]
wrote:

[image: woohoo]
https://cloud.githubusercontent.com/assets/1270126/12248899/47cd82a2-b871-11e5-835b-82a4776697f4.png

—
Reply to this email directly or view it on GitHub
#164 (comment)
.

andrewimm · 2016-01-11T23:25:41Z

cc @hallucinogen and @wangmengyan95 for review as well

hallucinogen · 2016-01-11T23:32:59Z

src/ParseObject.js

-    return Object.freeze(
-      ObjectState.estimateAttributes(this.className, this._getStateIdentifier())
-    );
+    let attributes = singleInstance ?


I think it would be better if we abstract this into InstanceStateController that can be configured by CoreManager.js

@hallucinogen I thought about that too, but they take completely different arguments -- one uses a set of strings as identifiers, the other uses an object. We could pass the Object and introspect the identifiers in most, but not all of the cases

andrewimm · 2016-01-12T00:42:07Z

So, we could probably abstract everything away and make this a controller, but one of the core reasons that ObjectStore exists, and why single-instance mode is addressable by a className + ID mapping, is to eventually publicize APIs that provide functional access over Parse Objects (a la Parse+React) without requiring any new code. Whether we consider the idea of SDK modularization or not, I like to think that it would be possible to build a reasonable lightweight application with just a Store, ParseOp, and RESTController.

I also recognize the value of shared interfaces, so here's what I think we should do. Rather than make the first argument a ParseObject (pulling in unnecessary dependencies), it should be an object that implements the interface { className: string, id: string }. Since a Parse Object implements this interface, we can always pass it as the first argument.

EDIT: actually we can't. Somehow I logic'd that incorrectly on my walk to the bus -- I guess it's actually an example of why I don't necessarily want a constant interface. Need to think about this more.

andrewimm · 2016-01-12T00:47:29Z

Continuing... another possible option is to replace the way we use _getStateIdentifier. Right now it returns the object ID; what if we used it to return the object I mention above instead?

In single instance mode, it returns an object of the shape { className: string, id: string }. In unique instance mode, it returns the ParseObject itself.

This is my favorite approach thusfar

grantland · 2016-01-12T01:32:57Z

This is going to be confusing, since the JS SDK is one level deeper than the other SDKs:

JS ParseObject
JS ObjectState == Android/iOS/.NET ParseObject
JS UniqueInstanceState/SingleInstanceState == Android/iOS/.NET ParseObject.State

Does ParseReact publicize ObjectState directly? If not, it'd be great if we can rename things to make it less confusing moving between codebases.

andrewimm · 2016-01-12T01:34:13Z

ObjectState is not documented, so we can rename things. I'd support renaming it, since it's really a set of state modifiers, not the state itself anymore.

hallucinogen · 2016-01-12T01:46:36Z

So, we could probably abstract everything away and make this a controller, but one of the core reasons that ObjectStore exists, and why single-instance mode is addressable by a className + ID mapping, is to eventually publicize APIs that provide functional access over Parse Objects (a la Parse+React) without requiring any new code.

It's still possible to eventually publicize APIs that provide functional access over ParseObjects (a.k.a publicize our current ObjectState) while still abstracting the State store. IMO, it's even more powerful if we abstract it as controller (and the API will allow people to build localStorage based State store! #randomidea #dontmindthis).

I agree with passing _getStateIdentifier.

grantland · 2016-01-12T01:49:33Z

Maybe ReactObject instead and ObjectState and Single(Instance)ObjectState?

andrewimm · 2016-01-12T19:01:18Z

Object State is now controlled by ObjectStateController, and swappable like any other controller. This should be good for expandability.

Also renamed ObjectState to ObjectStateMutations, since it's now a set of standardized mutation functions, and does not affect any state.

peterdotjs · 2016-01-12T20:11:33Z

src/SingleInstanceState.js

+export function getServerData(obj: ObjectIdentifier): AttributeMap {
+  let state = getState(obj);
+  if (state) {
+    return state.serverData;


Do we want to be consistent in the default return value for all these functions?
return state.serverData || {};
like we did in getState?

Can you explain further? Each function's "default state" has a pretty specific meaning that is leveraged in the code. In general, I don't think that's the bit that should be up for review now; those are the things that are remaining the same before & after this diff.

Oh, for your specific example, it's an invariant that state.serverData exists if state exists, check out the typedef at https://github.com/ParsePlatform/Parse-SDK-JS/pull/164/files#diff-5c0499aa286ed5bb8b6f6ad90b87830cR26

ah i missed that - looks like it's all covered then.

peterdotjs · 2016-01-12T20:13:02Z

Really like the extensibility now with ObjectStateController.

hallucinogen · 2016-01-12T21:00:49Z

src/CoreManager.js

+  setPendingOp: (obj: ParseObject, attr: string, op: ?Op) => void;
+  pushPendingState: (obj: ParseObject) => void;
+  popPendingState: (obj: ParseObject) => OpsMap;
+  mergeFirstPendingState: (obj: ParseObject) => void;


Hmm, you sure this is correct? mergeFirstPendingState of ObjectStateController accepts ParseObject.....

hallucinogen · 2016-01-12T21:01:34Z

src/ParseObject.js

  }

  _handleSaveError() {
    var pending = this._getPendingOps();
-    ObjectState.mergeFirstPendingState(this.className, this._getStateIdentifier());
+    let stateController = CoreManager.getObjectStateController();
+    stateController.mergeFirstPendingState(this._getStateIdentifier());


... meanwhile in practice, it accepts stateIdentifier....

hallucinogen · 2016-01-12T21:03:05Z

Loving it! One question though

andrewimm · 2016-01-12T23:28:17Z

I'll let @hallucinogen give the green light on this one.

hallucinogen · 2016-01-12T23:47:36Z

src/ParseObject.js

@@ -34,10 +33,12 @@ import {
 import ParsePromise from './ParsePromise';
 import ParseQuery from './ParseQuery';
 import ParseRelation from './ParseRelation';
+import * as SingleInstanceState from './SingleInstanceState';


I hope you won't hate me for this. But this should be SingleInstanceStateController. I feel strongly about this since SingleInstanceState sounds like model, while SingleInstanceStateController clearly... a controller.

nah, makes sense, though with every extra word I worry about things getting too Java-y. I'll go find-replace.

AbstractInstanceStateControllerFactory, here we come!

andrewimm · 2016-01-13T00:32:20Z

Boomdone. Really missing phab macros right about now

hallucinogen · 2016-01-13T00:33:17Z

src/CoreManager.js

@@ -207,6 +218,107 @@ module.exports = {
    return config['ObjectController'];
  },

+  setObjectStateController(controller: ObjectStateController) {
+    if (typeof controller.getState !== 'function') {
+      console.log(controller);


productioncleanup

hallucinogen · 2016-01-13T00:35:02Z

LGTM. One last nits, but accepting anyway.

andrewimm · 2016-01-13T00:35:33Z

tyvm

Split ObjectState into utilities and storage implementations

andrewimm added 4 commits January 11, 2016 09:00

Split ObjectState out into utilities and SingleInstanceState

a7b729d

Migrate ParseObject to use SingleInstanceState

8a7b2f4

Implement UniqueInstanceState

230f8f8

Let ParseObject and ParseUser use UniqueInstanceState

386610b

andrewimm assigned peterdotjs Jan 11, 2016

andrewimm added this to the 1.7.0 milestone Jan 11, 2016

facebook-github-bot added the CLA Signed label Jan 11, 2016

moar tests

fb1c397

hallucinogen reviewed Jan 11, 2016
View reviewed changes

andrewimm added 2 commits January 12, 2016 10:48

Restructure state implementations as swappable controllers

053eed3

Rename ObjectState to ObjectStateMutations

5002c27

andrewimm mentioned this pull request Jan 12, 2016

Release 1.7 #136

Closed

5 tasks

peterdotjs reviewed Jan 12, 2016
View reviewed changes

hallucinogen reviewed Jan 12, 2016
View reviewed changes

Fix a bunch of flow typing

3c061bf

hallucinogen reviewed Jan 12, 2016
View reviewed changes

Rename to state controllers

7d92864

hallucinogen reviewed Jan 13, 2016
View reviewed changes

hallucinogen added the accepted label Jan 13, 2016

hallucinogen assigned andrewimm and unassigned peterdotjs Jan 13, 2016

productioncleanup

f6d4c00

andrewimm added a commit that referenced this pull request Jan 13, 2016

Merge pull request #164 from ParsePlatform/singleinstancestore

26c016d

Split ObjectState into utilities and storage implementations

andrewimm merged commit 26c016d into release-1.7.0 Jan 13, 2016

andrewimm deleted the singleinstancestore branch January 25, 2016 18:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split ObjectState into utilities and storage implementations #164

Split ObjectState into utilities and storage implementations #164

andrewimm commented Jan 11, 2016

andrewimm commented Jan 11, 2016

andrewimm commented Jan 11, 2016

andrewimm commented Jan 11, 2016

roddylindsay commented Jan 11, 2016

TylerBrock commented Jan 11, 2016

andrewimm commented Jan 11, 2016

hallucinogen Jan 11, 2016

andrewimm Jan 11, 2016

andrewimm commented Jan 12, 2016

andrewimm commented Jan 12, 2016

grantland commented Jan 12, 2016

andrewimm commented Jan 12, 2016

hallucinogen commented Jan 12, 2016

grantland commented Jan 12, 2016

andrewimm commented Jan 12, 2016

peterdotjs Jan 12, 2016

andrewimm Jan 12, 2016

andrewimm Jan 12, 2016

peterdotjs Jan 12, 2016

peterdotjs commented Jan 12, 2016

hallucinogen Jan 12, 2016

hallucinogen Jan 12, 2016

hallucinogen commented Jan 12, 2016

andrewimm commented Jan 12, 2016

hallucinogen Jan 12, 2016

andrewimm Jan 13, 2016

andrewimm commented Jan 13, 2016

hallucinogen Jan 13, 2016

hallucinogen commented Jan 13, 2016

andrewimm commented Jan 13, 2016

Split ObjectState into utilities and storage implementations #164

Split ObjectState into utilities and storage implementations #164

Conversation

andrewimm commented Jan 11, 2016

andrewimm commented Jan 11, 2016

andrewimm commented Jan 11, 2016

andrewimm commented Jan 11, 2016

roddylindsay commented Jan 11, 2016

TylerBrock commented Jan 11, 2016

andrewimm commented Jan 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewimm commented Jan 12, 2016

andrewimm commented Jan 12, 2016

grantland commented Jan 12, 2016

andrewimm commented Jan 12, 2016

hallucinogen commented Jan 12, 2016

grantland commented Jan 12, 2016

andrewimm commented Jan 12, 2016

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

peterdotjs commented Jan 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hallucinogen commented Jan 12, 2016

andrewimm commented Jan 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewimm commented Jan 13, 2016

Choose a reason for hiding this comment

hallucinogen commented Jan 13, 2016

andrewimm commented Jan 13, 2016