Skip to content

[Fiber] Always push context before bailouts#1

Closed
gaearon wants to merge 2 commits intobvaughn:unexpected-context-popfrom
gaearon:bailout-bug
Closed

[Fiber] Always push context before bailouts#1
gaearon wants to merge 2 commits intobvaughn:unexpected-context-popfrom
gaearon:bailout-bug

Conversation

@gaearon
Copy link

@gaearon gaearon commented Dec 17, 2016

When I first wrote the context code, I didn't fully understand how bailouts work. Even if a fiber bails out, it still has to complete. Therefore we should always push the context even if we later bail out.

When I first wrote the context code, I didn't fully understand how bailouts work. Even if a fiber bails out, it still has to complete. Therefore we should always push the context even if we later bail out.
@bvaughn
Copy link
Owner

bvaughn commented Dec 17, 2016

This looks much nicer, having things grouped into a single function like that.

It looks like we're maybe pushing too frequently now though. For example, given a ClassComponent, it will push once as a result of pushContextIfNecessary and again inside of updateClassComponent (assuming it doesn't bail out early). Similarly a HostComponent will push as a result of pushContextIfNecessary as well as updateHostComponent.

Seems like an oversight?

@sebmarkbage
Copy link

This is the downside of colocating begin with other begins and complete with other completes instead of the type of work. Pattern matching vs. OO. :)

@gaearon
Copy link
Author

gaearon commented Dec 17, 2016

For example, given a ClassComponent, it will push once as a result of pushContextIfNecessary and again inside of updateClassComponent (assuming it doesn't bail out early)

Good catch. Can we just remove those remaining push calls and use this one? But I'm worried why tests didn't catch this.

Could you add corresponding tests that are failing with this PR, and cherry-pick my fix (or equivalent) and then fix up those remaining cases?

@bvaughn
Copy link
Owner

bvaughn commented Dec 17, 2016

Can we just remove those remaining push calls and use this one? But I'm worried why tests didn't catch this.

I think so. Yeah, that seems like a better way to go.

Could you add corresponding tests that are failing with this PR, and cherry-pick my fix (or equivalent) and then fix up those remaining cases?

Yeah. This will likely take me a while though so I'll probably do it on Monday or over the weekend (not tonight). Catching these cases in tests is a little tricky for me still. 😄

@gaearon
Copy link
Author

gaearon commented Dec 17, 2016

There are a couple more issues here.
I think I mostly figured them out but I'll open a separate PR against master when I'm done.

@gaearon gaearon closed this Dec 17, 2016
@gaearon gaearon deleted the bailout-bug branch December 17, 2016 11:14
bvaughn pushed a commit that referenced this pull request Aug 8, 2017
* Remove PooledClass from FallbackCompositionState

The only module that uses FallbackCompositonState is BeforeInputEventPlugin. The way its structured means there can only be a single instance of FallbackCompositionState at any given time (stored in a local variable at the top-level) so we don't really need pooling here at all. Instead, a single object is now stored in FallbackCompositionState, and access (initializing, reseting, getting data) is gaurded by the exported helper object.

* Use new FallbackCompositionState API in BeforeInputEventPlugin

* Implement event-specific pooling in SyntheticEvent

* Remove PooledClass from TopLevelCallbackBookKeeping

* Update results.json

* Add pooled event test fixtures (#1)

* Fix fixture lint
lunaruan pushed a commit that referenced this pull request Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants