Open k-g-a opened 5 years ago
I love the initiative of making MST faster! I ran into the same issue that aggregating data undoes any optimization that was done in terms of lazy loading.
Since my use case doesn't use much actions it won't fix my performance issues with MST. However, I agree that the addition of "self" to make things more Dev friendly does not weight up to the loss in performance.
Looking forward to your other performance-related RFCs.
While I love the idea of more speed, I can't think of any way to make TS get along with that properly without using self and a function :-/
However, maybe if self was actually a proxy that would change the current instance (this) based on over what object is the invoker then maybe the actions/views/etc could be reutilized for all the objects of the same kind?
.actions((self) => ({ // self is a proxy / object with defineProperty, which would be stored as type.self
incX() { self.x++ } // "cache" this action
})
someObj.incX()
// would do
type.self = someObj instance
type.actions.incX() // would increase type.self.x -> someObj.x
restore old type.self (if any)
But I'm not sure how well that'd work with async methods though, so in those cases once should save the current "self" in the top of the function, which is weird and error prone :-/
Another option would be to include self as part of the function params themselves, and that would allow plain objects to be passed
with({
incX: action((s) => {
s.x++
}),
xTimes2: view((s) => s.x * 2)
// viewFunction, volatile, etc
})
Also it would be good since it makes clear you cannot use self outside of them anymore. And also it shouldn't be too hard to make that pattern work in TS I think (as long as those helper functions (action, view, etc) are in place, that is).
Still, if done any of these ways there would need to be some kind of init((self) => {})
for models so they are able to do stuff which is usually done right now on the beginning of the block of "enhance/action/views/volatile" (a place to use addDisposer for effects, etc).
Actually I take it back, I found a way to make it work with TS :D (just discovered ThisType)
POC
interface Mdl<P, A> {
actions<A2>(a: ThisType<P & A & A2> & A2): Mdl<P, A & A2>;
create(): P & A;
}
function mdl<P>(p: P): Mdl<P, {}> {
return null as any;
}
const M = mdl({
x: 5
}).actions({
incX() {
this.addX(1)
},
addX(amount: number) {
this.x += amount
},
}).actions({
doubleX() {
this.x *= 2;
this.incX()
}
})
const m = M.create()
m.x
m.incX()
m.doubleX()
m.addX(1)
and it can be even typed so views can only access other views and props, but not actions
The good thing about being self-less is that there's no way to use a view that wasn't turned into a computed (since it has to go through "this" forcibly
Also I actually like how it forces separation from a possible init block / constructor-ish, wich could probably go like
const M = model({...})
// no this/self available, so it cannot call actions/views/etc, but can be used by them via this.localState
.localState(() => ({
// here stuff for initial non-observable local state, TS would use this for types
// can contain shared private methods, or plain variables...
// if used more than once then the type of local state is the & of the previous one and the next one
// akin to private class properties/methods
foo: "bar"
})) // the returned object will become part of (this/self).localState
// can use 'this' to access previous stuff
.volatile({...})
.views({...})
.actions({ ... })
// behaviors, no access to this, but access to self
.onMount((self, {onParentChange, addFx}) => {
// node is ready, created, and attached (unless it is a root node) (afterCreationFinalization)
// what most people would use
// effects could go here, e.g.
const fx1 = addDisposer(self, reaction(...));
// or
const fx2 = addFx(reaction(...));
// early fx disposal
fx1();
fx2();
// can use onParentChange if needed (similar to old attach/detach)
// would be called only on future parent changes after mounted
// won't get called upon destroy with newParent = null
onParentChange((newParent?, oldParent?) => {
...
})
// can return a disposer that would be like beforeDestroy
return () => {
...
};
})
// creation (touch) mode, eager or lazy
// in eager mode the node (and therefore its parent chain) will be "accessed" as soon as the root node is finalized
// in case of composition, where one model specifies 'lazy' and the other 'eager', eager wins
.creationMode('lazy' | 'eager')
would fit well with the new "hooks" idea I think. The only omission is 'onCreate' and 'onAttach' / 'onDetach', but I think finalization (onMount) and onParentChange is more clear
The reason I put onParentChange inside onMount is so state can be more easily shared between all behaviors (avoiding having to resort to localState as much as possible, in other words, localState would only be needed in the rare cases where a view/action/etc need to use something set in the behaviors)
@mweststrate I'd like to draw your attention to this RFC as you've been showing decent activity in this repo lately :) We're heavily using MST for our new project. For now memory/cpu loads are quite OK for us, but the amount of data will heavily increase in future, so I'd love to spend some time in optimizing MST around september-october.
@k-g-a I think it is a really good idea. Especially because we can introduce it gradually by introducing as experimental type first as you suggested.
As far as I know the typings should indeed be no problem thanks to ThisType
, I used that in similar situations in the past as well. In fact, this might be a little bit better resilient against self-referring types in TS, which are a pain now (not entirely sure, but it might be that TS can handle it this way better).
To be better able to optimize further in the future, I would keep the api really small and strict for now: So only support the object mode (not the thunks), and explicit API per hooks as suggested by @xaviergonz to create initialization / destruction blocks. I'd start with no more that that, as I'd expect that get's you really far already.
Really looking forward to the outcome!
Hi, all, It's been a year since this improvement was proposed- are there any plans for implementing it? Are there any branches or forks with a working version?
Hi! For now, performance is not a big concern for our project as the current implementation works fast enough, but this eventually has to end. I still want to introduce changes proposed by this RFC, but can not guarantee any time bounds. I will report back here as soon as we focus on performance
@k-g-a Would it be possible to post the proof of concept implementation? Would love to read that!
Hi, @BrianHung! Unfortunatelly I don't have any artifacts left of those experiments. The code was too far away from being appropriate for pushing somewhere.
The key idea was to define actions mostly the same way as views (model.ts
code instantiateViews
/instantiateActions
) and to bind those to self
instead of actions
. There also were some simplifications within createActionInvoker
. Finally the handler passed to actions
got type
instead of self
(instance) as an argument.
issue moved to: https://github.com/coolsoftwaretyler/mst-middlewares/issues/9
^ Please ignore, this is specifically related to MST. I think it got caught up in the issue transfer because it says the word "middleware". That's my bad for not being clear enough. Let's leave this open for now.
Problem
MST is known to be much less performant in terms of both CPU load and memory footprint than pure mobx. Currently the problem is partly solved by making everything as lazy as it can be. But the problem reappears as soon as one needs to aggregate some data across all the instances.
For the example of 50 Parents (4 props + children, 2 computeds, 4 actions) each holding 100 Children (2 props + children, 2 computeds, 2 actions) each holding 10 GrandChildren (2 props, 2 computeds, 1 action) - resulting in 60100 ObjectNodes + 110200 ScalarNodes:
There is no doubt that MST provides a good layer of convinience by adding strong typings (even runtime), bridge to immutable objects (snapshots/patches), middlewares, hooks and so on. And those benefits could not come without a cost. But we could try to put some efforts to minimize this cost.
Proposal
Although there are several ways to reduce overhead, the most efficient for now is to move
actions
from instance to prototype. The most significant impact of this cahnge is that there will be noself
withinactions
- the code inside actions must usethis
instead. I've made a proof of concept test withing my current project (> 40 different trees, with references, flows, middlewares etc.) and didn't meet a single issue with incorrectly boundthis
. Nevertheless, end-users will have to run a codemod or make by-hand codebase changes to swich fromself
tothis
- thats why I propose to release it under separate type (SelflessModel, FastModel, BoundModel - name to be discussed). As soon as we get positive feedback from users and eliminate possible bugs we can make this implementation default.For consistency purpose we should also allow use
this
withinfviews
/volatile
/extend
. We actually do forviews
, but do not encourage for now.Motivation
For my proof of concept implementation I had the following results:
Detailed design
Overview of proposed changes has the following look:
Model
;actions
/views
/volatile
/extend
accept plain objects or() => PLAIN_OBJECT
delegates, which usethis
inside;_actionDescriptors
,_viewDescriptors
viacloneAndEnhance
);instantiate
call a base object is constructed which holds all the actions wrapped increateActionInvoker
(which is currently called for every action on every instance) - this base object is used as prototype forobservableInstance
createActionInvoker
is slightly modified to usethis
instead oftarget
ascontext.context
; this one could be further refactored, but I'll describe it at separete RFC later on;ObjectNode
's_applyPatches
/_applySnapshot
are extracted frompreboot
and just bound at this point tostoredValue
(which is cheaper and fater, than recreating);Drawbacks
self
is positioned as one of the MST benefits, but this proposal discouragaesself
;I'm willing to make PR, if the idea is accepted. I'll also try to describe other performance-related RFCs withing a week or two.