[Need Design Review] Set up an experiment to share one lock across each entire embedding

[Adlai-Holler]

This diff works.

Basically when a node is initialized it needs a target "context." Node context might be a cell in a table, or a view controller, or some silo'd off area that we will prohibit users from changing for a given node.

If we're willing to accept this restriction, then the context can own a mutex which is an invariant, and which is pointed to by all the nodes in the context, and we can freely traverse the tree, go crazy, and do whatever we need to do during our transactions on the tree. Right now we are tippy-toeing around things that should be easy, like upward propagation of layout invalidation.

So first of all, please people take a look and give useful feedback.

One thing you'll notice is in the ASDisplayNode+*.mm files I have redefined __instanceLock__ as a macro that points to our _mutexOrPtr.get() which acts as the switch during experimentation. As soon as this lands, I will land a find-and-replace that removes those macros. It's just to keep this diff from being cluttered.

The end state is for all the major consumers to experiment with this, prove it works, then launch it and remove the forking. Nodes no longer own their own mutexes, all nodes have a context, boom.

[maicki]

I like the direction we are going here! One question that came up while looking over it is that if a developer would like to implement it's own container class does he need to know and handle the fact there is a context? We are currently providing support within our container classes (ASTableView, ASCollectionView and ASViewController), but if so it would add some burden on top of developers to understand and handle the existance of an ASNodeContext.

[maicki]

Do we need to expose the nodeContext publicly?

[Adlai-Holler]

To your question above, if users want to implement custom containers, and they want to add new nodes to a given context outside of our automatic places (like layout) they will need to push the context for an existing node. At least that's my thinking although I haven't written out an example case yet.

[Adlai-Holler]

Minor updates, just harder experiment gating and a build fix.

[bolsinga]

I like this direction. In the multi-threaded WebCore iPhone fork (all of 2 threads) back in 2007, there was a single lock for the entire DOM. It would be taken as needed by the web thread or the main thread. Maybe when you say there is one lock per "embedded" this is conceptually like the "node tree"?

Another possibility (something we did later around 2008 or 2009) was to allow each thread to take the web lock. Once either thread had it, it then held the lock until the "bottom" of the run loop. This is because we found we had tons of contention for the single lock. It was faster in the end to let each thread complete its work instead of bouncing around.

[bolsinga]

Is it possible to write tests for this behavior?

Are there many developers that have custom containers? I'm so unaware, my bet is the Pinterest is one of them. ;)

[Adlai-Holler]

@bolsinga Thanks for weighing in! Yep it's effectively a node tree, the only difference being somewhat pedantic – before & after a given layout is applied, the nodes in that layout may not be formally in the tree but they are still subject to the same lock. This is important because invalidating the layout of a random node that is part of a layout spec tree/yoga tree but isn't yet in the node tree should still be subject to the same serialization.

An open question that got raised elsewhere:

• What about node rehosting? This is a somewhat common technique for advanced transitions, wherein maybe a video goes fullscreen.
  • Option 1: Make node context mutable, where each node owns an atomic_shared_ptr and uses double-checked locking to, load the pointer, lock it, then load the pointer again. If the context changed during the lock, retry the process.
    • Significantly more complicated in terms of architecture and state-space.
    • More overhead. Two atomic loads plus the lock each time.
  • Option 2: Force rehosters to always rehost in the same context. So if you are creating a new cell, and rehosting the entire cell, just returning the existing cell is fine. If you want to rehost just one part of the cell tree, then the new cell you create should inherit the context of the original. It would not be an option to rehost a subnode into an existing tree from a different context.

To the question about custom embeddings, I'd like to see how this looks in practice (Pinterest indeed) but the thinking now is, you will need to call ASNodeContextPush/Pop around your node creation code. Doesn't seem from the outside to be too onerous.

Interesting to hear about the WebCore prior art here!

I won't land this before adding unit tests, indeed.

[bolsinga]

I think a first pass of using #2 sounds best, as keeping first steps simple is best. If it winds up being an onerous ordeal for clients, then building the library code to do it could be considered.

I also like for how large of a conceptual/runtime change this is, it is a surprisingly small diff. Thanks.

[ghost]

🚫 CI failed with log

[ghost]

🚫 CI failed with log

[ghost]

	1 Warning
⚠️	Please ensure license is correct for ASNodeContextTests.mm: // // ASNodeContextTests.mm // Texture // // Copyright (c) Pinterest, Inc. All rights reserved. // Licensed under Apache 2.0: http://www.apache.org/licenses/LICENSE-2.0

Generated by 🚫 Danger

[ghost]

🚫 CI failed with log

[nguyenhuy]

FYI, CI seems to hit legitimate errors.

[Adlai-Holler]

I have an improved version of this downstream. Let me upstream it and we'll see what people think

[bolsinga]

@Adlai-Holler Did a diff like this get into production? It looks like something we'd like to try out. Thanks.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}