Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-layout-api] Lifetime for instance of layout class. #746

Open
bfgeek opened this issue Apr 7, 2018 · 1 comment
Open

[css-layout-api] Lifetime for instance of layout class. #746

bfgeek opened this issue Apr 7, 2018 · 1 comment

Comments

@bfgeek
Copy link
Contributor

bfgeek commented Apr 7, 2018

In our prototype implementation we have the instance of the class - e.g.

registerLayout('foo', class {
  *layout() {
      this; // <- this object
   }
});

... the instance of the class tied to the box lifetime. I.e. it will get GC'd only after the box drops out of the box tree.

Do we want this behaviour? It is useful for caching things between layouts like what happens with the paint api.

@css-meeting-bot
Copy link
Member

The Working Group just discussed Lifetime policy for passed in objects, e.g. LayoutChild, and agreed to the following resolutions:

  • RESOLVED: The instance is tied to the same policy as worklet swapping.
The full IRC log of that discussion <dael> Topic: Lifetime policy for passed in objects, e.g. LayoutChild
<dael> github: https://github.com//issues/746
<dael> iank_: For a box with a layout instance it's tied to a lifetime of boxes. The box drops out of the tree.
<dael> dbaron: This is the thing in the spec with a bunch of prowse about these get chaced by worklet and only cache is distroyed when box is dropped.
<dbaron> s/prowse/prose/
<dbaron> s/chaced/cached/
<dael> iank_: Yes. For example in our impl if we have 2 layout level scopes both will have one instance. It lets you have expensive cached calcualtion = something
<dael> iank_: We could have a policy where we kill it every 1000 times and regenerate, but there's a value to keep them
<dael> dbaron: There's the same worry about layout global scopes...so if you're keeping worklet level every 1000 you'll get something after relayout.
<dael> iank_: Yes. Is the global scope switching enough? Or do we want something more expensive
<dael> Rossen: Will you be allowed to reuse the worklet?
<dael> iank_: Yes. If you've got 2 boxes that run the same worklet, each will have its own instance.
<dael> iank_: A good example is 3. In our impl we do specific caching on internals, you could do that here for layout reasons.
<dael> fremy: Didn't we discussion a shared cache across worklets?
<dael> iank_: We did....
<dael> Rossen: If what you want to share is the results...unless the read only cache is read/write that's okay. but then you will be producing a lot of generating. If it's read-only it has to be communiable. But that's a good first step.
<dael> Rossen: In more complex layouts you have a ton of layout information you want to preserve between passes.
<dael> iank_: It's regernatable.
<dael> iank_: That's why I prefer an instance that does live across many frames. Question is do we nuke this instance.
<dael> Rossen: If I understnad, the only cost we will have when we nuke the instance or somehow remove it, it's perf.
<dael> iank_: Yes. Should be cheap from our PoV to regen the state. That's the big question. This is somewhat edgecasey, but every minute need to recreate.
<dael> Rossen: When we discussed 2+ worklets and all the costs, we did this for many reasons. Why should we change those for this case? I would say for now carry the policy.
<dael> iank_: Policy doesn't require you to kill, just switch.
<dael> iank_: We're not wiping out a state we just have bad state.
<dael> Rossen: My hope is anything you do that depends on gloabl state should be discouraged. If we have an explicit reason we will care information on your behalf, that's fine. I don't see why this should be special unless we run into memory pressure. In that case you have to be prepared to regen.
<dael> iank_: Lifetime of the box unless UA needs to regen.
<dael> iank_: I'm also fine with that or jut adding something preemtively.
<dael> Rossen: If the stash will be not needed can't we make it explicit?
<dael> iank_: It is.
<dael> Rossen: But if you have a stach that's necessary...if they have a stash where these are the things I spent 40 minutes calculating the perfect size and I want to preserve it...I can have all kinds of other garbage you shouldn't retain.
<dael> iank_: But people that have cached, we can always be there later.
<dael> iank_: Fine with tying one time to the box?
<dael> TabAtkins: No worse then globals
<dael> Rossen: What is lifetime of the box?
<dael> s/one time/lifetime
<dael> emilio: Blink only dsitroys tree when display changes. So if you change display any parent box will be dropped.
<dael> iank_: Might be a reason why we nuke the instance every 1000 times. You drop the box on resizes?
<dael> Rossen: Yeah.
<dael> iank_: May be why we drop periodically
<dael> iank_: Reuse the instance no more then 1000 times as the policy?
<dael> dbaron: How can you reuse more?
<dael> iank_: Gloabl scopes changes, but don't tear down. Here we're saying you reached the max and we're going to ear it down and rebuild
<dael> dbaron: Might be good to have a sentence or two with explaining the algo.
<dael> iank_: Yeah.
<dael> dbaron: As a reader if there had been 2 sentences explaining the algo I wouldn't have had to read the alog.
<dbaron> s/alog/algorithm/
<dael> iank_: Reuse the layout instance no more than 1000 times, after that you must regin.
<dael> Rossen: Or drop whenever you switch worklets.
<surma> s/regin/regenerate/
<dael> Rossen: Other suggestions? prop: The instance is tied to the same policy as worklet swapping.
<dael> iank_: Not explicitly.
<dael> Rossen: If you switch event 1000 times you get it 1000 times.
<dael> iank_: I'm fine with that.
<dael> Rossen: Let's see how it goes. If perf is decremented then let's discuss it. It's always easier to relax later.
<dael> Rossen: Objections?
<dael> RESOLVED: The instance is tied to the same policy as worklet swapping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants