hello @nikomatsakis sorry to bother you, I was looking at this and wanted to check: to check which variables refer to which, I should do a mir walk, looking for statements / assign ?
@lqd yes, I think you could use a MIR visitor
do you by any chance remember an area of the compiler where we look for similar kinds of information, so that I don't bother you for trivial questions ?
(mostly to see the kinds of visit callbacks I'd need to look for, but I guess, assignments, statements, maybe locals and places, etc)
hmm, I can't think of anything quite comparable -- but I imagined that you would probably visit assignments
and then match on the rvalue
alright thank you, it'll be a good start already
alrighty, here's a quick WIP try/hack? at finding which locals flow into statics . Is this direction how you envisioned it or am I off base ?
and if that's approximately ok, how would I then go about forcing the escaping locals' regions to outlive
(as another data point, completely disabling liveness in our
html5ever big static benchmark case ended up improving around 25% so it's likely the upper bound of this future fix, and it's also likely that time would now go into dataflow/moveck like we recently talked about wrt to
@lqd I am off to the beach just now ;) but from a 22-second glance looks right
will try to look more closely later
thanks :) enjoy the beach :beach_with_umbrella:
@lqd left a few comments now
thanks for the comments, I'll try to do as much as I can before leaving :)
I had started previously in the way the comments say, and testing on the "mini html5ever" version it seemed like it would stop a bit before getting to the big inner arrays. The MIR was indeed very similar to the
const X: u32 = add(&22, &44); case except the references were to these big arrays instead, and my thoughts were that most of the liveness time was spent computing it for the array's elements, which I assumed I wouldn't prevent with this initial way. I'll expand checking to projections as well, and check the rvalues conservatively :)
(maybe the bigger wins / biggest time spent may be coming from the behaviour that is shared with
tuple-stress in this benchmark :thinking_face: )
@lqd when do you leave btw?
are you saying there were fn calls in the MIR?
that said, I agree this will not be enough to "solve" html5ever...
I'm looking at the MIR for the smaller variant now (playground)
it seems like we need aggregates, ref expressions (
&foo), unsizing casts, moves...
and that's it?
leaving tomorrow so timing is tight :)
oh no it wasn’t the function calls, just refs to inputs and I likely misunderstood these shouldn’t escape — but It seems the eg “_1 = const 22u32” shouldn’t escape but “_2 = &1” should, and how I did it absolutely accepts the inputs when it shouldn’t
specifically it wasn't immediately clear to me in the comment's example playground how the MIR for the static X was to be handled differently from the static NAMED_ENTITIES
ok now I think I understand the comment sorry :/ by looking at the rvalue directly I wouldn't mark locals which are visited as byproducts of eg the const fn call, which I surely get rn. I was looking at the MIR for the inputs while the problem would appear in the output...
@lqd if you wind up leaving we can always "hand off" the branch
yeah I hope I can finish this first step of computing the escaping locals during the night, so it’s at least done before handing it off to incorporate into liveness
pushed this commit which should be more in-line with how we wanted to do the analysis; in my tests it seemed to correctly analyze the
html5ever static, and detect all but the const fn call in our counter-example. There are things I lacked time to do/wasn't sure exactly under which form we wanted them:
- this needs tests
- maybe debug logs of the escaping locals gathering
- the big one I expected would not have enough time to finish: it still doesn't do anything with the escaping locals to filter them from liveness.
I can still do some fixups in the morning btw. Still, sorry I couldn't push this over the finish line :/
but otherwise, I think it should be in good enough shape to hand off
@lqd awesome, I have some plane rides coming up, maybe I'll pick it up
@nikomatsakis safe travels niko :)
@lqd what branch? (I was going to pull it locally and get the build process started)
(the fork is a bit behind, like 6 days)
(@David Wood whistling + you're a brit right ? :D)
(@lqd yeah, Scottish)
@lqd this is not a unique cultural reference ;-) I'd say many (most?) nerds would get it.
I love making fun branch names. They are usually non-obvious
@Jake Goulding :)
hi, I can continue work on this issue if nobody on it rightnow
I'm still not able to really be online, but I did poke at this branch a bit actually @mikhail-m1 and I think I kind of got it working
argh I just realized a huge bug in this implementation -- the idea is sound, but overly aggressive
opened https://github.com/rust-lang/rust/pull/53109 but I'll post some notes for how to do this properly
@lqd or @mikhail-m1 still interested?
I left a comment here https://github.com/rust-lang/rust/issues/52713#issuecomment-410699523
Hi @nikomatsakis , I don't mind to continue work on it, if @lqd don't have time...
I predict I will be pretty caught up :)
take a look at my comment — I think it's not a major change that is needed
@mikhail-m1 shall I assign to you then?
I'm leaning towards this alternative approach now
I can also take a stab (as you prefer)
I had some other ideas for perf improvements I need to try and write up
@mikhail-m1 I'm still away until next week, feel free to take it :)
@nikomatsakis yes, you can assign it to me, but it will take some time to understand how to implement the new approach
@mikhail-m1 if you can give me some high-level tips of what might need explanation, I can try to add more details
@nikomatsakis yes, it will be helpful
ok let me try to leave comments that are a bit more in depth. As far as delays, seems ok, whenever possible I always prefer to mentor! =) Last week I just happened to be sort of unavailable ...
@mikhail-m1 i'm going to leave a few notes now