Closed Bug 1367904 Opened 8 years ago Closed 7 years ago

stylo: fuse ServoComputedValues and nsStyleContext into a single allocation

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: bzbarsky, Assigned: manishearth)

References

(Blocks 2 open bugs)

Details

Attachments

(16 files, 20 obsolete files)

Bug 1367904 - Part 16: stylo: Stop using mStyleIfVisited in Servo;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1367904 - Part 13: stylo: Flatten ServoComputedValues into ServoStyleContext;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1367904 - Part 14: stylo: Remove FFI calls for fetching style structs from ServoComputedValues;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1367904 - Part 15: stylo: Override ArenaRefPtr for ServoStyleContexts;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1367904 - Part 1: stylo: Create C++-side ServoComputedValues;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1367904 - Part 2: stylo: Add RawOffsetArc<T>;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1367904 - Part 3: stylo: Use RawOffsetArc in ComputedValues;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1367904 - Part 4: stylo: Replace real ComputedValues with bindgenned ComputedValues2;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1367904 - Part 5: stylo: Make GetBaseComputedValuesForElement return a style context;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1367904 - Part 6: stylo: Introduce ComputedValuesInner;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1367904 - Part 7: stylo: Use ComputedValuesInner instead of ComputedValues when we don't need it;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1367904 - Part 9: stylo: Make Servo Arc types use ptr to T instead of ptr to ArcInner<T>;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1367904 - Part 10: stylo: Switch Gecko over to ServoStyleContext;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1367904 - Part 11: stylo: Use ServoStyleContext in ServoStyleSet/ServoRestyleManager;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1367904 - Part 17: stylo: Clean up GetContext, call it UpdateWithElementState;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1367904 - Part 18: stylo: Mark visited failures as expected;
59 bytes, text/x-review-board-request
jryans
: review+
Details
There should be no need for FFI here. We have a pointer to the ArcInner<ComputedValues> already, afaict. We should be able to add sizeof(usize) to get a pointer to the ComputedValue, then add the right offset to get to the right Arc inside there, dereference to get the new ArcInner<StructType>, add sizeof(usize), and return that. Needs some repr(C), of course, plus some way to find the relevant offsets, but if we just line up the struct order in Gecko and Servo that should be simple.
Yes, I've been thinking about this. We'll want to make StyleArc repr(C). I think we should then change stylearc::Arc's point to point directly to T, rather than to the ArcInner<T>, and move the pointer arithmetic from Deref to Clone. That way, the pointers will point straight to the structs, which will make it easier to use them from C++.
We'll then probably want to define ComputedValues in C++, and use bindgen (along with type substitution) to generate the Rust ComputedValues representation.
So the way I was considering doing this was as follows: 1) Make stylearc::Arc repr(C). 2) Make ArcInner repr(C). 3) Switch the order of members in the ArcInner so T comes before the refcount. The effect of #3 is precisely to make the Arc point directly to the T in practice. Then we can drop all the "add sizeof(usize)" above, and getting a struct becomes just "take our C++-side pointer, add the offset of the struct, dereference, return the result". I hadn't quite decided what the best approach was for ComputedValues.
(In reply to Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-) from comment #3) > So the way I was considering doing this was as follows: > > 1) Make stylearc::Arc repr(C). > 2) Make ArcInner repr(C). > 3) Switch the order of members in the ArcInner so T comes before the > refcount. That also seems doable, though I think it might prevent dynamically-sized types from ever being used inside a stylearc. We don't need that, but if possible I'd like to take the approaches with stylearc that are most likely to be upstream-able. Worth checking with Rust folks to see if they see any deal-breakers with the "point one word into the ArcInner" approach.
An alternative is to leave the stylearc::Arc pointer and the structs as they are, and have into_raw/from_raw methods (and perhaps also variations that don’t take ownership of a refcount?) that do the address offset and return/take *const T: https://doc.rust-lang.org/std/sync/struct.Arc.html#method.into_raw https://doc.rust-lang.org/std/sync/struct.Arc.html#method.from_raw
It's doable, sure; it's just more instructions to get to the thing we want.
(In reply to Simon Sapin (:SimonSapin) from comment #5) > An alternative is to leave the stylearc::Arc pointer and the structs as they > are, and have into_raw/from_raw methods (and perhaps also variations that > don’t take ownership of a refcount?) that do the address offset and > return/take *const T: How does that help? The point here is that we want the style struct to be repr(C) and inspectable directly by C++. We could write that pointer-munging in C++, but it's dicey because we don't have access to the types, and it's more instructions anyway (per comment 6).
We can have style structs be repr(C) without making (what is now called) servo_arc::ArcInner that contain them also repr(C). Also, you just added usage of servo_arc::Arc with dynamically-sized types for selectors. If I understand correctly that requires the refcount (and rest of statically-sized header data) to come first in the memory layout.
(In reply to Simon Sapin (:SimonSapin) from comment #8) > We can have style structs be repr(C) without making (what is now called) > servo_arc::ArcInner that contain them also repr(C). I should have said "we want the struct containing the style struct pointers to be repr(C)". > Also, you just added > usage of servo_arc::Arc with dynamically-sized types for selectors. If I > understand correctly that requires the refcount (and rest of > statically-sized header data) to come first in the memory layout. I'm not suggesting adjusting the layout of ArcInner. I'm suggesting that we adjust the the pointer inside the servo_arc::Arc (which, as of yesterday, is a NonZeroPtrMut) such that it points 1 word into the beginning of the allocation. In effect, this shifts the burden of pointer arithmetic from Deref to Clone/Drop (and their associated munging of the refcount). But more importantly, it allows C++ to traverse the pointers, because it can assume that the pointer inside and Arc<T> points directly to a T.
I'll write up a patch to demonstrate what I mean.
I understand what you mean now. Either way works for me.
I got stuck on the fact that we have Arc<T: ?Sized>, and I can't figure out how to write offset_of logic that works on T? sized (which may or may not be fat). Arc::{from_raw,into_raw} get around this by just requiring Sized, but we can't have different internal data representations depending on whether we're Sized or not. Any ideas?
Thinking about this more, I think this might be better solved more concretely in ComputedValues.
I just profiled this, and the overhead over Servo_GetStyle* is actually huge on wikipedia - on the order of 50ms. It's less on other sites, but still significant. Some of that might be cache missing on the ComputedValues, which wouldn't be solved by inlining the access. But we could solve that by fusing the allocations of nsStyleContext and ComputedValues. Anyway, I'll work on eliminating the FFI calls for starters.
Assignee: nobody → bobbyholley
Priority: -- → P1
So I've been thinking about this, and getting very enthusiastic about the fusing strategy alluded to above. Here's how it would work: (1) Implement "thin" nsStyleContexts as described in bug 1367863 comment 1, and stop presarena-allocating the ServoStyleContext instances. This can happen as a separate bug, which I can probably knock it out quick-ish. (2) Add RawArc<T> wrapper, similar to ThinArc<T>, which is repr(C) and wraps a T* but otherwise behaves like an Arc. I have mostly-finished patches for this, which I can post shortly. (3) Replace all the style struct Arc<T> instances in ComputedValues with RawArc<T>. (4) Add a C++ ServoRawArc<T> type, which dereferences to a T*, and which bindgen knows how to replace with a RawArc<T> on the Rust side. (5) Modify ServoStyleContext to store all the style structs inline as ServoRawArc<T>. as well as the random other fields on ComputedValues. (6) Eliminate ComputedValues, and store ServoStyleContext directly. There is a never-ending stream of goodness that would flow from this: * No FFI calls to get the style structs. * No extra pointer chase (and cache miss) when getting computed style for a frame. * No extra memory and fragmentation overhead of having two heap-allocated data structures. * No performance overhead of doing the extra allocations. * nsStyleContext allocations happen in parallel, rather than sequentially. * No more gymnastics to get the 'previous style context for restyle damage'. * Once we do this, we can _potentially_ eliminate the post-traversal entirely, and update the frame style contexts directly from rayon (though this would be a separate followup bug). The key thing that allows this is the elimination of the style context tree state from the style context, specifically the child pointers. I had always thought we couldn't do this until we dropped the old style system, but now realize we can, with the subclassing approach in (1). Manish is going to work on this.
Assignee: bobbyholley → manishearth
Summary: stylo: Figure out a way to not do FFI calls for Style* getters on nsStyleContext → stylo: fuse ServoComputedValues and nsStyleContext into a single allocation
Blocks: 1367863
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #15) > So I've been thinking about this, and getting very enthusiastic about the > fusing strategy alluded to above. > > Here's how it would work: > (1) Implement "thin" nsStyleContexts as described in bug 1367863 comment 1, > and stop presarena-allocating the ServoStyleContext instances. This can > happen as a separate bug, which I can probably knock it out quick-ish. > (2) Add RawArc<T> wrapper, similar to ThinArc<T>, which is repr(C) and wraps > a T* but otherwise behaves like an Arc. I have mostly-finished patches for > this, which I can post shortly. > (3) Replace all the style struct Arc<T> instances in ComputedValues with > RawArc<T>. > (4) Add a C++ ServoRawArc<T> type, which dereferences to a T*, and which > bindgen knows how to replace with a RawArc<T> on the Rust side. > (5) Modify ServoStyleContext to store all the style structs inline as > ServoRawArc<T>. as well as the random other fields on ComputedValues. > (6) Eliminate ComputedValues, and store ServoStyleContext directly. > > There is a never-ending stream of goodness that would flow from this: > * No FFI calls to get the style structs. > * No extra pointer chase (and cache miss) when getting computed style for a > frame. > * No extra memory and fragmentation overhead of having two heap-allocated > data structures. > * No performance overhead of doing the extra allocations. > * nsStyleContext allocations happen in parallel, rather than sequentially. > * No more gymnastics to get the 'previous style context for restyle damage'. > * Once we do this, we can _potentially_ eliminate the post-traversal > entirely, and update the frame style contexts directly from rayon (though > this would be a separate followup bug). It's not clear to me how... How do we plan to populate the change list with this setup?
(In reply to Emilio Cobos Álvarez [:emilio] from comment #16) > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #15) > > So I've been thinking about this, and getting very enthusiastic about the > > fusing strategy alluded to above. > > > > Here's how it would work: > > (1) Implement "thin" nsStyleContexts as described in bug 1367863 comment 1, > > and stop presarena-allocating the ServoStyleContext instances. This can > > happen as a separate bug, which I can probably knock it out quick-ish. > > (2) Add RawArc<T> wrapper, similar to ThinArc<T>, which is repr(C) and wraps > > a T* but otherwise behaves like an Arc. I have mostly-finished patches for > > this, which I can post shortly. > > (3) Replace all the style struct Arc<T> instances in ComputedValues with > > RawArc<T>. > > (4) Add a C++ ServoRawArc<T> type, which dereferences to a T*, and which > > bindgen knows how to replace with a RawArc<T> on the Rust side. > > (5) Modify ServoStyleContext to store all the style structs inline as > > ServoRawArc<T>. as well as the random other fields on ComputedValues. > > (6) Eliminate ComputedValues, and store ServoStyleContext directly. > > > > There is a never-ending stream of goodness that would flow from this: > > * No FFI calls to get the style structs. > > * No extra pointer chase (and cache miss) when getting computed style for a > > frame. > > * No extra memory and fragmentation overhead of having two heap-allocated > > data structures. > > * No performance overhead of doing the extra allocations. > > * nsStyleContext allocations happen in parallel, rather than sequentially. > > * No more gymnastics to get the 'previous style context for restyle damage'. > > * Once we do this, we can _potentially_ eliminate the post-traversal > > entirely, and update the frame style contexts directly from rayon (though > > this would be a separate followup bug). > > It's not clear to me how... How do we plan to populate the change list with > this setup? You're talking about the setup up "removing the post-traversal"? In general, that's out of scope for this bug, so we shouldn't really get into it here. We'd need to think about it, but it seems plausible to me that we could collect them on the ThreadLocalStyleContext somehow.
This is what I have so far. Most of it has been split, except for mCachedResetData/mCachedInheritedData and a couple of methods. Basically, ready to start going through review, while I finish up the rest. I'm not happy with how certain small calls can't be inlined right now due to how header files work; I may replace the AsGecko() definitions with stuff using stronger casts (currently they use static_cast, which requires it to be a subclass, but we don't know that it is a subclass until later and can't forward-declare this relationship). This doesn't do the actual fusing, but it can land independently as its own chunk (may slightly impact stylo perf since the context won't be arena allocated anymore).
> but we don't know that it is a subclass until later and can't forward-declare this relationship You can put the inline impl in a nsStyleContextInlines.h file, no?
> You can put the inline impl in a nsStyleContextInlines.h file, no? I could, except they're used by template methods in nsStyleContext.h, and I don't think those can be forward declared.
Template methods can be forward-declared, last I checked.
Depends on: 1373018
Attachment #8877472 - Attachment is obsolete: true
Attachment #8877472 - Flags: review?(bobbyholley)
Attachment #8877471 - Attachment is obsolete: true
Attachment #8877471 - Flags: review?(bobbyholley)
Attachment #8877470 - Attachment is obsolete: true
Attachment #8877470 - Flags: review?(bobbyholley)
Attachment #8877469 - Attachment is obsolete: true
Attachment #8877469 - Flags: review?(bobbyholley)
Attachment #8877468 - Attachment is obsolete: true
Attachment #8877468 - Flags: review?(bobbyholley)
Attachment #8877467 - Attachment is obsolete: true
Attachment #8877467 - Flags: review?(bobbyholley)
Attachment #8877466 - Attachment is obsolete: true
Attachment #8877466 - Flags: review?(bobbyholley)
Attachment #8877465 - Attachment is obsolete: true
Attachment #8877465 - Flags: review?(bobbyholley)
Blocks: 1373056
Blocks: 1373430
Blocks: 1373512
Attachment #8877465 - Attachment is obsolete: false
Attachment #8877466 - Attachment is obsolete: false
Attachment #8877467 - Attachment is obsolete: false
Attachment #8877468 - Attachment is obsolete: false
Attachment #8877469 - Attachment is obsolete: false
Attachment #8877470 - Attachment is obsolete: false
Attachment #8877471 - Attachment is obsolete: false
Attachment #8877472 - Attachment is obsolete: false
Attachment #8877465 - Flags: review?(bobbyholley)
Attachment #8877466 - Flags: review?(bobbyholley)
Attachment #8877467 - Flags: review?(bobbyholley)
Attachment #8877468 - Flags: review?(bobbyholley)
Attachment #8877469 - Flags: review?(bobbyholley)
Attachment #8877470 - Flags: review?(bobbyholley)
Attachment #8877471 - Flags: review?(bobbyholley)
Attachment #8877472 - Flags: review?(bobbyholley)
Attachment #8877465 - Attachment is obsolete: true
Attachment #8877465 - Flags: review?(bobbyholley)
Attachment #8877466 - Attachment is obsolete: true
Attachment #8877466 - Flags: review?(bobbyholley)
Attachment #8877467 - Attachment is obsolete: true
Attachment #8877467 - Flags: review?(bobbyholley)
Attachment #8877468 - Attachment is obsolete: true
Attachment #8877468 - Flags: review?(bobbyholley)
Attachment #8877469 - Attachment is obsolete: true
Attachment #8877469 - Flags: review?(bobbyholley)
Attachment #8877470 - Attachment is obsolete: true
Attachment #8877470 - Flags: review?(bobbyholley)
Attachment #8877471 - Attachment is obsolete: true
Attachment #8877471 - Flags: review?(bobbyholley)
Attachment #8877472 - Attachment is obsolete: true
Attachment #8877472 - Flags: review?(bobbyholley)
Attachment #8878668 - Attachment is obsolete: true
This just creates the machinery, without using it. Nowhere near complete, but you can start reviewing if you want. Or wait for me to let you know when it's all done.
Comment on attachment 8878669 [details] Bug 1367904 - Part 16: stylo: Stop using mStyleIfVisited in Servo; https://reviewboard.mozilla.org/r/149970/#review154766 ::: layout/style/ServoTypes.h:149 (Diff revision 1) > + > +struct ServoVisitedStyle { > + uintptr_t mPtr; > +}; > + > +struct ServoComputedValues2 { I assume we'll eventually merge this with the other ServoComputedValues type? ::: layout/style/ServoTypes.h:150 (Diff revision 1) > +struct ServoVisitedStyle { > + uintptr_t mPtr; > +}; > + > +struct ServoComputedValues2 { > +#define STYLE_STRUCT(name_, checkdata_cb_) nsStyle##name_* name_; I think we should probably mPrefix these names for consistency, so mFont instead of just Font. ::: layout/style/ServoTypes.h:153 (Diff revision 1) > + ServoCustomPropertiesMap custom_properties; > + ServoWritingMode writing_mode; > + ServoFontComputationData font_computation_data; > + ServoVisitedStyle visited_style; I think these should be mPrefixed and camel-cased. ::: layout/style/ServoTypes.h:157 (Diff revision 1) > +#undef STYLE_STRUCT > + ServoCustomPropertiesMap custom_properties; > + ServoWritingMode writing_mode; > + ServoFontComputationData font_computation_data; > + ServoVisitedStyle visited_style; > + ~ServoComputedValues2() {} // do nothing, but prevent Copy from being impl'd by bindgen Even better would be to mark it private, omit the {}, and never define the dtor, so that we can't destroy it from C++.
Attachment #8878669 - Flags: review?(bobbyholley) → review+
Comment on attachment 8878670 [details] Bug 1367904 - Part 2: stylo: Add RawOffsetArc<T>; https://reviewboard.mozilla.org/r/149972/#review154768 ::: servo/components/servo_arc/lib.rs:710 (Diff revision 1) > +fn offset_to_normal<T: 'static>(offset: *mut T) -> *mut ArcInner<T> { > + let fake_inner_align: usize = mem::align_of::<ArcInner<T>>(); > + let fake_inner_ptr = fake_inner_align as *const ArcInner<T>; > + let fake_t_ptr = unsafe { &(*fake_inner_ptr).data } as *const T; > + debug_assert!(fake_t_ptr as usize > fake_inner_ptr as usize); > + let diff = fake_t_ptr as usize - fake_inner_ptr as usize; > + debug_assert!(offset as usize > diff); > + unsafe { offset.offset(- (diff as isize)) as *mut ArcInner<T> } > +} Hm, why do we need all this stuff? Can't we just re-use the existing logic for Arc::{from,into}_raw? That's what the partial patches I had for this did, IIRC. ::: servo/components/servo_arc/lib.rs:770 (Diff revision 1) > + /// Converts an Arc into a ThinArc. This consumes the Arc, so the refcount > + /// is not modified. This comment and the one below should refer to RawOffsetArc, not ThinArc.
Added more patches. We now distinguish in Servo/Stylo between ComputedValues and ComputedValuesInner and pick which one to use appropriately. I'm not yet sure I've picked the right one to use in all cases, review appreciated. We'll also need to use RefPtr<CV> or RawOffsetArc<CV> in Stylo instead of Arc, so I made one step to set the stage for that. I may also introduce a ComputedValuesBorrowed type which can be obtained from a ComputedValuesStrong which is effectively a reference to the inner T but is known to be a reference obtained from a RefPtr (and thus can be treated as a RefPtr, temporarily). I'd prefer to use RefPtr, not RawOffsetArc, here, because Gecko code already assumes that nsStyleContext is RefPtrd, and the implicit servo-side refcount field will be tricky to deal with.
If possible, we might want to land these patches first, to avoid bitrot. They ought to be idempotent behavior-wise.
I'm not really happy with part 9. As discussed on IRC, the problem is that this bug will make ServoStyleContext to be the first Servo-managed Arc type that is not opaque on the Gecko side. Currently `RefPtr<T>` (when servo-managed) holds an opaque pointer to the allocation, not a pointer to the T. But here we will want to use nsStyleContext* to actually mean a pointer to the style context, which would not work. There are two ways of handling this -- continue Gecko-managing ServoStyleContext (i.e. the allocation and refcounting is done by gecko and via FFI on servo) and use RefPtr on the Servo side (we already have sugar for this), or make it so that HasArcFFI provides offset pointers for the gecko side (i.e. it effectively uses RawOffsetArc instead of Arc). After discussion we agreed that the second option was better, and that has been implemented in part 9. I'm not really happy with that patch now that I've written it; it adds extra steps to the safety wrappers, making their use further contorted (which is kind of defeating their point). I expected some of this but not as much as that patch needed. Part of this is because before this change we could always convert an &Arc to &RawServoFooBorrowed and vice versa, but now it's not possible because one is offset and there is nothing to take a pointer of. This is why ArcBorrow needs to exist -- for when we want a pointer to an Arc-like thing but can't take a pointer to the actual Arc itself. (It's also doing a weird segfault currently, which I haven't yet debugged. But I'm not fond of piling this amount of unsafe risk in a single bug). The original design of HasArcFFI was back when we didn't have stylearc, so it wasn't actually possible to have something like RawOffsetArc or ArcBorrow. Now that we have these, I suspect it's possible to clean this up in a better way, but it will involve sprinkling the style codebase (including common code) with a lot of RawOffsetArc and ArcBorrow. It will also be a major refactor that really should be out of scope for this bug. Given that doing this well involves sprinkling what really should be Gecko-specific types (RawOffsetArc/ArcBorrow) throughout the codebase anyway, may I instead try the RefPtr way? Because of Part 8, this is actually a minor change that won't visibly affect the code much. It will mean that allocating ServoStyleContext will require a bit of a servo-gecko dance, but we need that anyway to run the right constructors.
Comment on attachment 8880262 [details] Bug 1367904 - Part 9: stylo: Make Servo Arc types use ptr to T instead of ptr to ArcInner<T>; https://reviewboard.mozilla.org/r/151616/#review156690 ::: servo/components/servo_arc/lib.rs:863 (Diff revision 1) > + > + // Expose the transient Arc to the callback, which may clone it if it wants. > + let result = f(&transient); > + > + // Forget the transient Arc to leave the refcount untouched. > + mem::forget(transient); Why do you need to `forget` this? It's a `NoDrop`, right?
Comment on attachment 8880262 [details] Bug 1367904 - Part 9: stylo: Make Servo Arc types use ptr to T instead of ptr to ArcInner<T>; https://reviewboard.mozilla.org/r/151616/#review156690 > Why do you need to `forget` this? It's a `NoDrop`, right? IIRC there's a panic involved on some implementations of NoDrop. As unions stabilize it will go away.
Comment on attachment 8878670 [details] Bug 1367904 - Part 2: stylo: Add RawOffsetArc<T>; https://reviewboard.mozilla.org/r/149972/#review156838 ::: servo/components/servo_arc/lib.rs:707 (Diff revision 2) > +/// > +/// This means that this is a direct pointer to > +/// its contained data (and can be read from by both C++ and Rust), > +/// but we can also convert it to a "regular" Arc<T> by removing the offset > +pub struct RawOffsetArc<T: 'static> { > + ptr: *mut T, We should use NonZeroPtrMut here so that the NonZero optimization applies. ::: servo/components/servo_arc/lib.rs:752 (Diff revision 2) > + // Forget the transient Arc to leave the refcount untouched. > + mem::forget(transient); Emilio's point about NoDrop here is a good one. Let's add a comment indicating that, and then remove the mem::forget. Might as well do that for the other with_arc() implementation as well.
Attachment #8878670 - Flags: review?(bobbyholley) → review+
Comment on attachment 8878671 [details] Bug 1367904 - Part 3: stylo: Use RawOffsetArc in ComputedValues; https://reviewboard.mozilla.org/r/149974/#review156842
Attachment #8878671 - Flags: review?(bobbyholley) → review+
(In reply to Manish Goregaokar [:manishearth] from comment #56) > Comment on attachment 8880262 [details] > Bug 1367904 - Part 9: stylo: Make Servo Arc types use ptr to T instead of > ptr to ArcInner<T>; > > https://reviewboard.mozilla.org/r/151616/#review156690 > > > Why do you need to `forget` this? It's a `NoDrop`, right? > > IIRC there's a panic involved on some implementations of NoDrop. As unions > stabilize it will go away. Oh, I didn't see this. Ignore the last bit of comment 57 then.
(In reply to Manish Goregaokar [:manishearth] from comment #56) > Comment on attachment 8880262 [details] > Bug 1367904 - Part 9: stylo: Make Servo Arc types use ptr to T instead of > ptr to ArcInner<T>; > > https://reviewboard.mozilla.org/r/151616/#review156690 > > > Why do you need to `forget` this? It's a `NoDrop`, right? > > IIRC there's a panic involved on some implementations of NoDrop. As unions > stabilize it will go away. Can you comment on it, and perhaps with a link to the relevant issue?
Comment on attachment 8878698 [details] Bug 1367904 - Part 4: stylo: Replace real ComputedValues with bindgenned ComputedValues2; https://reviewboard.mozilla.org/r/150006/#review156852 ::: servo/components/servo_arc/lib.rs:208 (Diff revision 3) > + pub fn refcount(&self) -> usize { > + self.inner().count.load(Acquire) > + } Err, what? Why do we need this? I don't see it used anywhere, and in general exposing the refcount of Arcs doesn't seem great. ::: servo/components/style/properties/properties.mako.rs:2188 (Diff revision 3) > + pub fn clone_arc<T: 'static>(x: &BuilderArc<T>) -> Arc<T> { > + Arc::from_raw_offset(x.clone()) Does this actually need to be pub? Same as below.
Attachment #8878698 - Flags: review?(bobbyholley) → review+
Comment on attachment 8879797 [details] Bug 1367904 - Part 5: stylo: Make GetBaseComputedValuesForElement return a style context; https://reviewboard.mozilla.org/r/151180/#review156856
Attachment #8879797 - Flags: review?(bobbyholley) → review+
Comment on attachment 8878669 [details] Bug 1367904 - Part 16: stylo: Stop using mStyleIfVisited in Servo; https://reviewboard.mozilla.org/r/149970/#review154766 > I assume we'll eventually merge this with the other ServoComputedValues type? Yes; basically we already have a ServoComputedValues in the bindings and I don't want to confuse the two until other things get renamed.
Comment on attachment 8879798 [details] Bug 1367904 - Part 6: stylo: Introduce ComputedValuesInner; https://reviewboard.mozilla.org/r/151182/#review156860
Attachment #8879798 - Flags: review?(bobbyholley) → review+
Comment on attachment 8879799 [details] Bug 1367904 - Part 7: stylo: Use ComputedValuesInner instead of ComputedValues when we don't need it; https://reviewboard.mozilla.org/r/151184/#review156882 Would have been easier to review this with the layout renames in a separate patch, but the rest of this patch is great so I can't complain too much. r=me with the placeholder thing handled. ::: servo/ports/geckolib/glue.rs:1442 (Diff revision 2) > if data.get_styles().is_none() { > warn!("Calling Servo_ResolvePseudoStyle on unstyled element"); > return if is_probe { > Strong::null() > } else { > - doc_data.borrow().default_computed_values().clone().into_strong() > + doc_data.borrow().default_computed_values().clone().to_outer().into_strong() So this adds an extra allocation where we previously just had an Arc bump, but it's in a condition we should assert against anyway, so I'm ok with it. ::: servo/ports/geckolib/glue.rs:1538 (Diff revision 2) > } else { > debug_assert!(!for_text); > data.default_computed_values().clone() > }; > > - style.into_strong() > + style.to_outer().into_strong() Hm. This, on the other hand, seems much less optimal. Previously we always just shared the original allocation of the default values for placeholder frames (which pass null as the parent [1]) whereas now we'll heap-allocate a new one every time. However, it is true that, when we start allocating fused objects, we can't just blindly re-use the default ComputedValues, because placeholder frames are tagged with a specific anonymous box (oofPlaceholder). So how about the following: (1) change the assertion from just asserting against text nodes to asserting that we're resolving for placeholder. (2) Add a lazily-instantiated cached_placeholder_style: Option<Arc<ComputedValues>> alongside the default computed values. We should set it to None whenever the default computed values change. (3) Use that cache here. Doing this as a separate patch (in the same bug) is fine/preferable. [1] http://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/layout/style/ServoStyleSet.cpp#538 ::: servo/ports/geckolib/glue.rs:2555 (Diff revision 2) > > if !valid_styles { > debug_assert!(false, "Resolving style on element without current styles with lazy \ > computation forbidden."); > let per_doc_data = PerDocumentStyleData::from_ffi(raw_data).borrow(); > - return per_doc_data.default_computed_values().clone().into_strong(); > + return per_doc_data.default_computed_values().clone().to_outer().into_strong(); Given the debug_assert!(false), I'm fine with this one too.
Attachment #8879799 - Flags: review?(bobbyholley) → review+
Comment on attachment 8879812 [details] Bug 1367904 - Part 8: stylo: Replace Arc<ComputedValues> with StrongComputedValues so we can use a different refptr later; https://reviewboard.mozilla.org/r/151206/#review156904
Attachment #8879812 - Flags: review?(bobbyholley) → review+
(In reply to Manish Goregaokar [:manishearth] from comment #54) > I'm not really happy with part 9. > > As discussed on IRC, the problem is that this bug will make > ServoStyleContext to be the first Servo-managed Arc type that is not opaque > on the Gecko side. > > Currently `RefPtr<T>` (when servo-managed) holds an opaque pointer to the > allocation, not a pointer to the T. But here we will want to use > nsStyleContext* to actually mean a pointer to the style context, which would > not work. > > There are two ways of handling this -- continue Gecko-managing > ServoStyleContext (i.e. the allocation and refcounting is done by gecko and > via FFI on servo) and use RefPtr on the Servo side (we already have sugar > for this), or make it so that HasArcFFI provides offset pointers for the > gecko side (i.e. it effectively uses RawOffsetArc instead of Arc). > > After discussion we agreed that the second option was better, and that has > been implemented in part 9. > > I'm not really happy with that patch now that I've written it; it adds extra > steps to the safety wrappers, making their use further contorted (which is > kind of defeating their point). I expected some of this but not as much as > that patch needed. Part of this is because before this change we could > always convert an &Arc to &RawServoFooBorrowed and vice versa, but now it's > not possible because one is offset and there is nothing to take a pointer > of. This is why ArcBorrow needs to exist -- for when we want a pointer to an > Arc-like thing but can't take a pointer to the actual Arc itself. What about my suggestion from yesterday to make all the FFI Arcs use RawOffsetArc, so that &RawServoFooBorrowed would be castable as &RawOffsetArc<T>? > (It's also doing a weird segfault currently, which I haven't yet debugged. > But I'm not fond of piling this amount of unsafe risk in a single bug). Like I said yesterday, converting all our ArcFFI stuff to RawOffsetArc could land independently underneath this set. > > The original design of HasArcFFI was back when we didn't have stylearc, so > it wasn't actually possible to have something like RawOffsetArc or > ArcBorrow. Now that we have these, I suspect it's possible to clean this up > in a better way, but it will involve sprinkling the style codebase > (including common code) with a lot of RawOffsetArc and ArcBorrow. It will > also be a major refactor that really should be out of scope for this bug. > > Given that doing this well involves sprinkling what really should be > Gecko-specific types (RawOffsetArc/ArcBorrow) throughout the codebase > anyway, may I instead try the RefPtr way? Because of Part 8, this is > actually a minor change that won't visibly affect the code much. It will > mean that allocating ServoStyleContext will require a bit of a servo-gecko > dance, but we need that anyway to run the right constructors. So here are my beefs with the RefPtr way: The current refcount stuff on nsStyleContext is 32-bit and non-threadsafe. So we're already going to need to hoist that into GeckoStyleContext, and then have nsStyleContext::{AddRef,Release} switch on IsGecko()/IsServo() and delegate to {Gecko,Servo}StyleContext::{AddRef,Release}. So we could then add NS_INLINE_DECL_THREADSAFE_REFCOUNTING to ServoStyleContext. But then we have the question of what ~ServoStyleContext does, because it doesn't have the type information to actually tear down the allocation from C++. So then we need an FFI call back to Servo to actually do the destroying, which is an extra FFI call than we'd otherwise have and generally feels very fragile. It also differs from the setup we have with the style structs (allocate on the Rust side, placement new/destroy over FFI), which adds complexity. Moreover, the biggest reason I want to keep the allocation in Rust (which I forgot to mention yesterday) is that it keeps the door open for a concurrent arena allocator, which would use the same mechanism to allocate ServoStyleContexts as it would for style structs. This is something we may need to do at some point (we're waiting to see how much of the overhead we can eliminate before doing that).
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #65) > Comment on attachment 8879799 [details] > Bug 1367904 - Part 7: stylo: Use ComputedValuesInner instead of > ComputedValues when we don't need it; > > https://reviewboard.mozilla.org/r/151184/#review156882 > > Would have been easier to review this with the layout renames in a separate > patch, but the rest of this patch is great so I can't complain too much. > r=me with the placeholder thing handled. > > ::: servo/ports/geckolib/glue.rs:1442 > (Diff revision 2) > > if data.get_styles().is_none() { > > warn!("Calling Servo_ResolvePseudoStyle on unstyled element"); > > return if is_probe { > > Strong::null() > > } else { > > - doc_data.borrow().default_computed_values().clone().into_strong() > > + doc_data.borrow().default_computed_values().clone().to_outer().into_strong() > > So this adds an extra allocation where we previously just had an Arc bump, > but it's in a condition we should assert against anyway, so I'm ok with it. > > ::: servo/ports/geckolib/glue.rs:1538 > (Diff revision 2) > > } else { > > debug_assert!(!for_text); > > data.default_computed_values().clone() > > }; > > > > - style.into_strong() > > + style.to_outer().into_strong() > > Hm. This, on the other hand, seems much less optimal. Previously we always > just shared the original allocation of the default values for placeholder > frames (which pass null as the parent [1]) whereas now we'll heap-allocate a > new one every time. > > However, it is true that, when we start allocating fused objects, we can't > just blindly re-use the default ComputedValues, because placeholder frames > are tagged with a specific anonymous box (oofPlaceholder). > > So how about the following: > (1) change the assertion from just asserting against text nodes to asserting > that we're resolving for placeholder. > (2) Add a lazily-instantiated cached_placeholder_style: > Option<Arc<ComputedValues>> alongside the default computed values. We should > set it to None whenever the default computed values change. > (3) Use that cache here. > > Doing this as a separate patch (in the same bug) is fine/preferable. Nevermind! Emilio pointed out that we already have such a cache on the C++ side: http://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/layout/style/ServoStyleSet.cpp#541 So this patch is good to go as-is without modifications.
Figured out the segfault, I'd forgotten to addref in ArcBorrow::clone_arc().
Comment on attachment 8880262 [details] Bug 1367904 - Part 9: stylo: Make Servo Arc types use ptr to T instead of ptr to ArcInner<T>; https://reviewboard.mozilla.org/r/151616/#review157296 lgtm modulo the moz.build thing. Thanks! ::: toolkit/library/moz.build:67 (Diff revision 2) > OS_LIBS += ['atomic'] > > # This option should go away in bug 1290972, but we need to wait until > # Rust 1.12 has been released. > if CONFIG['OS_ARCH'] == 'Darwin': > - LDFLAGS += ['-Wl,-no_compact_unwind'] > + LDFLAGS += ['-Wl,-no_compact_unwind,-lresolv'] Hm, what's this about? If we need it, please make it a separate patch, and flag froydnj for review.
Attachment #8880262 - Flags: review?(bobbyholley) → review+
It's an existing patch from a different bug that makes Gecko build on the latest Rust. It's already landing, so the next time I rebase this it will disappear from these patches.
Comment on attachment 8880262 [details] Bug 1367904 - Part 9: stylo: Make Servo Arc types use ptr to T instead of ptr to ArcInner<T>; https://reviewboard.mozilla.org/r/151616/#review157304 I think Bobby's review is enough here, I'd have to go through all the other patches to make sense of all this. I have a few nits (the same one repeteadly, actually), and I'm not in love with the extra verbosity, but I guess... ::: servo/components/style/gecko/wrapper.rs:1365 (Diff revision 2) > VisitedHandlingMode::RelevantLinkVisited => unsafe { > Gecko_GetVisitedLinkAttrDeclarationBlock(self.0) > }, > }; > - let declarations = declarations.and_then(|s| s.as_arc_opt()); > + let declarations: Option<&RawOffsetArc<Locked<PropertyDeclarationBlock>>> > + = declarations.and_then(|s| s.as_arc_opt()); nit: I think we usually put the equals in the previous line.
Attachment #8880262 - Flags: review?(emilio+bugs)
Comment on attachment 8878669 [details] Bug 1367904 - Part 16: stylo: Stop using mStyleIfVisited in Servo; https://reviewboard.mozilla.org/r/149970/#review154766 > I think these should be mPrefixed and camel-cased. These are ultimately going to be used only on the servo side so I'd prefer them to be Rust style.
Attachment #8879812 - Attachment is obsolete: true
No longer blocks: 1373056
Comment on attachment 8878670 [details] Bug 1367904 - Part 2: stylo: Add RawOffsetArc<T>; https://reviewboard.mozilla.org/r/149972/#review159272 ::: servo/components/servo_arc/lib.rs:760 (Diff revision 5) > + > + // Expose the transient Arc to the callback, which may clone it if it wants. > + let result = f(&transient); > + > + // Forget the transient Arc to leave the refcount untouched. > + mem::forget(transient); This still needs a comment about why it's needed, if only to be aware about when to remove it.
Comment on attachment 8878669 [details] Bug 1367904 - Part 16: stylo: Stop using mStyleIfVisited in Servo; https://reviewboard.mozilla.org/r/149970/#review154694 ::: layout/style/ServoTypes.h:157 (Diff revision 1) > +#undef STYLE_STRUCT > + ServoCustomPropertiesMap custom_properties; > + ServoWritingMode writing_mode; > + ServoFontComputationData font_computation_data; > + ServoVisitedStyle visited_style; > + ~ServoComputedValues2() {} // do nothing, but prevent Copy from being impl'd by bindgen nit: There's a bindgen annotation for this. ::: layout/style/ServoTypes.h:12 (Diff revision 4) > #ifndef mozilla_ServoTypes_h > #define mozilla_ServoTypes_h > > #include "mozilla/TypedEnumBits.h" > > +#define STYLE_STRUCT(name_, checkdata_cb_) struct nsStyle##name_;; nit: There's a stray semicolon here. ::: layout/style/ServoTypes.h:136 (Diff revision 4) > +struct ServoWritingMode { > + uint8_t mBits; > +}; > + > +struct ServoFontComputationData { > + // 8 bytes, but is done as 4+4 for alignment Is there any way to make this less fragile, or at least static-assert we do the right thing? (Disregard this if it's done in other patch) ::: layout/style/ServoTypes.h:149 (Diff revision 4) > + > +struct ServoVisitedStyle { > + uintptr_t mPtr; > +}; > + > +struct ServoComputedValues2 { Why `ServoComputedValues2`? The name seems... fun. Also, this needs at least a general "why is this here" comment.
Comment on attachment 8878698 [details] Bug 1367904 - Part 4: stylo: Replace real ComputedValues with bindgenned ComputedValues2; https://reviewboard.mozilla.org/r/150006/#review159276 ::: layout/style/ServoBindings.h:71 (Diff revision 6) > class nsStyleCoord; > struct nsStyleDisplay; > class nsXBLBinding; > > +namespace mozilla { > + #define STYLE_STRUCT(name_, checkdata_cb_) struct Gecko##name_ {nsStyle##name_ gecko;}; I guess at this point we could drop the `gecko: Foo` from both sides. Is there any point on keeping it? ::: servo/components/servo_arc/lib.rs:195 (Diff revision 6) > let ptr = (ptr as *const u8).offset(-offset_of!(ArcInner<T>, data)); > Arc { > p: NonZeroPtrMut::new(ptr as *mut ArcInner<T>), > } > } > + nit: Remove this stray whitespace. ::: servo/components/servo_arc/lib.rs:209 (Diff revision 6) > // `ArcInner` structure itself is `Sync` because the inner data is > // `Sync` as well, so we're ok loaning out an immutable pointer to these > // contents. > unsafe { &*self.ptr() } > } > + pub fn refcount(&self) -> usize { (I think this was said before, but this isn't needed nor used).
Comment on attachment 8879798 [details] Bug 1367904 - Part 6: stylo: Introduce ComputedValuesInner; https://reviewboard.mozilla.org/r/151182/#review159280 ::: servo/components/style/properties/gecko.mako.rs:81 (Diff revision 5) > > -pub use ::gecko_bindings::structs::mozilla::ServoComputedValues2 as ComputedValues; > +pub use ::gecko_bindings::structs::mozilla::ServoComputedValues2 as ComputedValuesInner; > + > +#[derive(Clone, Debug)] > +pub struct ComputedValues { > + pub inner: ComputedValuesInner This needs docs. Why is it needed? I see nothing either here or in the commit message, and I find this super-confusing (and ugly).
Comment on attachment 8880262 [details] Bug 1367904 - Part 9: stylo: Make Servo Arc types use ptr to T instead of ptr to ArcInner<T>; https://reviewboard.mozilla.org/r/151616/#review159282 ::: layout/style/ServoStyleSet.cpp:1158 (Diff revision 4) > { > RefPtr<ServoComputedValues> cv = Servo_StyleSet_GetBaseComputedValuesForElement(mRawSet.get(), > aElement, > &Snapshots(), > aPseudoType).Consume(); > - return ServoStyleContext::Create(nullptr, presContext, aPseudo, > + return ServoStyleContext::Create(nullptr, aPresContext, aPseudoTag, This doesn't belong to this patch, right? ::: servo/components/servo_arc/lib.rs:197 (Diff revision 4) > p: NonZeroPtrMut::new(ptr as *mut ArcInner<T>), > } > } > > + /// Produce a pointer to the data that can be converted back > + /// to an arc Please mention that this is our version of `&Arc<T>` (and also why does it need to exist, because it seems somewhat silly if you don't think through it).
Bulk of the refactoring is done; mostly had lots of assertions and stuff. There's still one assertion being caused because one of the pseudo contexts in `<q>hi</q>` isn't getting the pseudo type set, which I need to investigate. We also have some assertions because visited style should be getting fresh style contexts. This needs to be fixed in to_outer, though actually the visited style stuff is largely extraneous now and can be removed (might do it in a followup) After I handle those two, the remaining work is to just make the ServoComputedValues inline (this ought to be easy since the heavy lifting was done in Part 10 already) and then to modify style struct getters to punch through the now-pure-C++ pointers. Turns out that I was able to avoid needing to keep around RefPtr<ServoComputedValues> entirely. We do pay the cost of constructing a full context over SCV in some canvas code but it's not that big a difference of overhead. Currently the only RefPtr<ServoComputedValues> is in ServoStyleContext itself and that will be flattened once the other bugs are sorted out. There are also a bunch of followup cleanups -- e.g. GetContext() doesn't need to exist in its current form anymore. We can also cut down on AsServo() calls in ServoStyleSet and ServoRestyleManager by making more use of ServoStyleContext.
Assertion and visited issue fixed. We used to always recreate visited style contexts, and now we still do (though it's clearer that this happens). This is suboptimal IMO, but I'm not clear on how exactly all this fits together so I haven't touched this yet.
Comment on attachment 8884616 [details] Bug 1367904 - Part 10: stylo: Switch Gecko over to ServoStyleContext; https://reviewboard.mozilla.org/r/155490/#review160508 ::: layout/base/ServoRestyleManager.cpp:433 (Diff revision 2) > ServoRestyleManager::ProcessPostTraversal(Element* aElement, > nsStyleContext* aParentContext, > ServoRestyleState& aRestyleState) > { > nsIFrame* styleFrame = nsLayoutUtils::GetStyleFrame(aElement); > + ServoStyleContext* parent = aParentContext ? aParentContext->AsServo() : nullptr; nit: Just change the type of the argument? ::: layout/base/ServoRestyleManager.cpp:539 (Diff revision 2) > > auto pseudo = aElement->GetPseudoElementType(); > nsIAtom* pseudoTag = pseudo == CSSPseudoElementType::NotPseudo > ? nullptr : nsCSSPseudoElements::GetPseudoAtom(pseudo); > > - newContext = aRestyleState.StyleSet().GetContext( > + RefPtr<ServoStyleContext> tempContext = Wait, what? I thought the whole purpose of this patch was removing this very bit... Care to explain? ::: layout/base/nsCSSFrameConstructor.cpp:1930 (Diff revision 2) > // We don't do this for pseudos that may trigger animations or transitions, > // since those need to be kicked off by the traversal machinery. > - bool isServo = pseudoStyleContext->IsServo(); > bool hasServoAnimations = false; > - if (isServo) { > - ServoComputedValues* servoStyle = pseudoStyleContext->ComputedValues(); > + ServoStyleContext * servo = pseudoStyleContext->GetAsServo(); > + if (servo) { nit: the star should move to the left. ::: layout/style/ServoBindings.cpp:218 (Diff revision 2) > + // because it is within an Arc it is unsafe for the Rust side to ever > + // carry around a mutable non opaque reference to the context, so we > + // cast it here. > + ServoStyleContext* parent = const_cast<ServoStyleContext*>(aParentContext); > + nsPresContext* pres = const_cast<nsPresContext*>(aPresContext); > + new (aContext) ServoStyleContext(parent, pres, aPseudoTag, aPseudoType, aValues.Consume()); nit: You can use `KnownNotNull` here. ::: layout/style/ServoStyleSet.cpp:264 (Diff revision 2) > { > MOZ_ASSERT(aContent->IsElement()); > Element* element = aContent->AsElement(); > > - RefPtr<ServoComputedValues> computedValues; > + RefPtr<ServoStyleContext> computedValues; > + ServoStyleContext* parent = aParentContext? aParentContext->AsServo() : nullptr; I think you should just change the type of the argument here, and use the FORWARD macros instead. ::: layout/style/ServoStyleSet.cpp:317 (Diff revision 2) > - if (visitedComputedValues) { > + if (resultIfVisited) { > parentIfVisited = aParentContext; > } > } > > + ServoStyleContext* parentIfVisitedServo = parentIfVisited ? parentIfVisited->AsServo() : nullptr; Same for all these. ::: layout/style/ServoStyleSet.cpp:326 (Diff revision 2) > bool relevantLinkVisited = isLink ? isVisitedLink : > (aParentContext && aParentContext->RelevantLinkVisited()); > > - RefPtr<ServoStyleContext> result = > - ServoStyleContext::Create(aParentContext, mPresContext, aPseudoTag, aPseudoType, > - computedValues.forget()); > + if (resultIfVisited) { > + RefPtr<ServoStyleContext> visitedContext = > + Servo_StyleContext_NewContext(resultIfVisited->ComputedValues(), parentIfVisitedServo, nit: Alignment. But, mind elaborating into why this is needed? Avoiding this was also the point of the patch IIUC. ::: layout/style/ServoStyleSet.cpp:571 (Diff revision 2) > > MOZ_ASSERT(aType < CSSPseudoElementType::Count); > > - RefPtr<ServoComputedValues> computedValues; > + RefPtr<ServoStyleContext> computedValues; > + > + nsIAtom* pseudoTag = nsCSSPseudoElements::GetPseudoAtom(aType); Why can't the caller do this if needed? ::: layout/style/ServoStyleSet.cpp:579 (Diff revision 2) > computedValues = Servo_ResolveStyle(aPseudoElement, mRawSet.get(), > mAllowResolveStaleStyles).Consume(); > } else { > const ServoComputedValues* parentStyle = > aParentContext ? aParentContext->ComputedValues() : nullptr; > + ServoStyleContext* parent = aParentContext? aParentContext->AsServo() : nullptr; nit: spacing around `?`. ::: layout/style/ServoStyleSet.cpp:586 (Diff revision 2) > Servo_ResolvePseudoStyle(aOriginatingElement, > aType, > + pseudoTag, > /* is_probe = */ false, > parentStyle, > + parent, Passing both `parentStyle` and `parent` feels quite wrong. Mind elaborating on why is it needed? ::: layout/style/ServoStyleSet.cpp:637 (Diff revision 2) > UpdateStylistIfNeeded(); > > bool skipFixup = > nsCSSAnonBoxes::AnonBoxSkipsParentDisplayBasedStyleFixup(aPseudoTag); > > - const ServoComputedValues* parentStyle = > + ServoStyleContext* parent = aParentContext? aParentContext->AsServo() : nullptr; nit: spacing around `?` too. Here and everywhere else. ::: servo/ports/geckolib/glue.rs:1455 (Diff revision 2) > } > > #[no_mangle] > pub extern "C" fn Servo_ResolvePseudoStyle(element: RawGeckoElementBorrowed, > pseudo_type: CSSPseudoElementType, > + pseudo_tag: *mut nsIAtom, As far as I can tell, all this passing pseudo-tag around should go away (it's not only ugly, but it's also just equivalent to `PseudoElement::from_pseudo_type(pseudo_type).map_or(ptr::null(), |p| p.atom())`.
> nit: Alignment. But, mind elaborating into why this is needed? Avoiding this was also the point of the patch IIUC. > > Wait, what? I thought the whole purpose of this patch was removing this very bit... Care to explain? As I said in comment 106, the work of removing the inner refptr is still there. Patch 10 is a refactoring that makes that step much easier; it avoids making behavioral changes but does change how the code is structured and does all the heavy lifting necessary to make the inlining work. > (it's not only ugly, but it's also just equivalent to Gecko passes around pseudo tags everywhere so I wasn't sure what I should do (again, avoiding potential behavioral changes). Is it ever possible for there to be a tag with a NotPseudo type or vice versa? If so, why do the constructors and functions everywhere pass around both? > Passing both `parentStyle` and `parent` feels quite wrong. Mind elaborating on why is it needed? ProbePseudoElementStyle. Again, trying to avoid too many behavioral changes in this patch because it's a big patch. > nit: Just change the type of the argument? See comment 106, fixing up ServoRestyleManager and ServoStyleSet to exclusively use ServoStyleContext is a followup. I may do it in this bug itself, but not in patch 10. ----------- I'll leave a comment when I'm actually *done*, till then please understand that my main focus here is to split the commits up to be the smallest possible things that still build and work. To be clear, current todos (in rough order of how I plan to fix it) - Make part 10 not break Servo compilation - Actually flatten SSC - Fix assertions - Avoid most AsServo() calls by using ServoStyleContext* everywhere - Remove one of the two copies of visited style - Move GetContext's behavior into the constructor or something
(In reply to Manish Goregaokar [:manishearth] from comment #110) > Is it ever possible for there to be a tag with a NotPseudo type or vice > versa? If so, why do the constructors and functions everywhere pass around > both? I don't think we ever have NotPseudo and a non-null tag. Note that the anonymous box pseudos all map to (Non)InheritingAnonBox and all the XUL tree pseudos to XULTree. We could just merge all of these into CSSPseudoElementType. I have a feeling that we used to support any pseudo-element that started with "::-moz-tree-" but after bug 1359205 it might be that we'd assert if we used anything other than those listed in nsCSSAnonBoxList.h anyway.
(In reply to Cameron McCormack (:heycam) from comment #114) > I don't think we ever have NotPseudo and a non-null tag. Note that the > anonymous box pseudos all map to (Non)InheritingAnonBox and all the XUL tree > pseudos to XULTree. We could just merge all of these into > CSSPseudoElementType. I have a feeling that we used to support any > pseudo-element that started with "::-moz-tree-" but after bug 1359205 it > might be that we'd assert if we used anything other than those listed in > nsCSSAnonBoxList.h anyway. (Didn't read anything before, but this seems to be related to my work in bug 1348488.) Do we ever support tree pseudo-elements? It seems to me those need some extra work because they are functional, so I assume we've never supported them properly in Stylo yet.
No, we haven't supported any XUL tree pseudos in stylo yet, but this is in the context of refactoring the C++ style context objects where we do.
Blocks: 1379830
Comment on attachment 8884616 [details] Bug 1367904 - Part 10: stylo: Switch Gecko over to ServoStyleContext; https://reviewboard.mozilla.org/r/155490/#review160950 ::: layout/style/ServoStyleSet.cpp:280 (Diff revision 2) > -ServoStyleSet::GetContext(already_AddRefed<ServoComputedValues> aComputedValues, > +ServoStyleSet::GetContext(already_AddRefed<ServoStyleContext> aComputedValues, > nsStyleContext* aParentContext, > nsIAtom* aPseudoTag, > CSSPseudoElementType aPseudoType, > Element* aElementForAnimation) Can we assert here that aPseudoTag and aPseudoType match the ones on the passed-in ServoStyleContext? ::: layout/style/ServoStyleSet.cpp:296 (Diff revision 2) > isLink = nsCSSRuleProcessor::IsLink(aElementForAnimation); > isVisitedLink = nsCSSRuleProcessor::GetContentState(aElementForAnimation) > .HasState(NS_EVENT_STATE_VISITED); > } > > - RefPtr<ServoComputedValues> computedValues = Move(aComputedValues); > + // XXXManishearth this is wrong! Elaborate? ::: layout/style/ServoStyleSet.cpp:515 (Diff revision 2) > } > > already_AddRefed<nsStyleContext> > ServoStyleSet::ResolveStyleForFirstLetterContinuation(nsStyleContext* aParentContext) > { > - const ServoComputedValues* parent = > + ServoStyleContext* parent = aParentContext? aParentContext->AsServo() : nullptr; Nit: Space after the ?. But why do we need to null-check where we didn't before? ::: layout/style/nsStyleContext.h:88 (Diff revision 2) > + if (mozilla::ServoStyleContext* servo = GetAsServo()) { > + Servo_StyleContext_AddRef(servo); > + return 1; > + } Two beefs with the reference counting setup here: (1) We should move the actual implementations of the reference counting to the subclasses, so that RefPtr<ServoStyleContext> will just invoke the right AddRef/Release routine directly rather than having to do a dynamic check for the type. Then, nsStyleContext::{AddRef,Release} can do the dynamic check and invoke the appropriate subclass routine. (2) The |return 1| is super scary. I don't think any callers actually use the return value here (RefPtr doesn't), so we should just change the signature to return void. ::: layout/style/nsStyleContext.cpp:174 (Diff revision 2) > + if (NS_IsMainThread()) { > - // Free any ImageValues we were holding on to for CSS variable values. > + // Free any ImageValues we were holding on to for CSS variable values. > + // not threadsafe > - CSSVariableImageTable::RemoveAll(this); > + CSSVariableImageTable::RemoveAll(this); > + } else { > + // leak otherwise :( > + // XXXManishearth figure out what to do here > + } I don't think any of our codepaths cause us to add things to the table, so you should be able to just make this gecko-only rather than checking the thread. That said, I'm not sure if we need to build some equivalent to this table for stylo. Please file a bug and NI heycam to ask. ::: layout/style/nsStyleContext.cpp:494 (Diff revision 2) > +void > +nsStyleContext::SetStyleIfVisited(already_AddRefed<nsStyleContext> aStyleIfVisited) > +{ Were there any changes to this function? I can't tell. ::: servo/components/style/stylist.rs:761 (Diff revision 2) > - self.quirks_mode).to_outer(); > + self.quirks_mode).to_outer(self.device(), parent_style_context, > + Some(pseudo_info.clone())); Nit: wonky indentation, here and below. ::: servo/ports/geckolib/glue.rs:1447 (Diff revision 2) > cascade_flags.insert(SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP); > } > let metrics = get_metrics_provider_for_product(); > - data.stylist.precomputed_values_for_pseudo(&guards, &pseudo, maybe_parent, > - cascade_flags, &metrics) > + data.stylist.precomputed_values_for_pseudo(&guards, &pseudo, parent_style_or_null.map(|x| &**x), > + cascade_flags, &metrics, > + (pseudo_tag, structs::CSSPseudoElementType_InheritingAnonBox), Why do we know this is an InheritingAnonBox? ::: servo/ports/geckolib/glue.rs:1477 (Diff revision 2) > return if is_probe { > Strong::null() > } else { > - doc_data.default_computed_values().clone().to_outer().into_strong() > + doc_data.default_computed_values() > + .clone().to_outer(doc_data.stylist.device(), parent_style_context, > + Some((pseudo_tag, pseudo_type))).into_strong() Indentation. ::: servo/ports/geckolib/glue.rs:1609 (Diff revision 2) > &pseudo, > rule_inclusion, > base, > is_probe, > - &metrics) > + &metrics, > + pseudo_info.clone(), parent_style_context) Wrapping. ::: servo/ports/geckolib/glue.rs:2705 (Diff revision 2) > if !valid_styles { > debug_assert!(false, "Resolving style on element without current styles with lazy \ > computation forbidden."); > let per_doc_data = PerDocumentStyleData::from_ffi(raw_data).borrow(); > - return per_doc_data.default_computed_values().clone().to_outer().into_strong(); > + return per_doc_data.default_computed_values().clone().to_outer(per_doc_data.stylist.device(), > + None, None).into_strong(); Indentation. ::: layout/base/ServoRestyleManager.cpp:503 (Diff revision 5) > if (displayContentsNode) { > oldStyleContext = displayContentsNode->mStyle->AsServo(); > } > } > > - RefPtr<ServoComputedValues> computedValues = > + RefPtr<ServoStyleContext> computedContext = Nit: The name "computedContext" seems like the worst of both worlds. Let's call it newStyleContext. ::: layout/base/nsCSSFrameConstructor.cpp:1930 (Diff revision 5) > // We don't do this for pseudos that may trigger animations or transitions, > // since those need to be kicked off by the traversal machinery. > - bool isServo = pseudoStyleContext->IsServo(); > bool hasServoAnimations = false; > - if (isServo) { > - ServoComputedValues* servoStyle = pseudoStyleContext->ComputedValues(); > + ServoStyleContext * servo = pseudoStyleContext->GetAsServo(); > + if (servo) { Nit: Let's continue calling this servoStyle. ::: layout/style/ServoBindingList.h:86 (Diff revision 5) > SERVO_BINDING_FUNC(Servo_StyleSet_GetCounterStyleRule, nsCSSCounterStyleRule*, > RawServoStyleSetBorrowed set, nsIAtom* name) > SERVO_BINDING_FUNC(Servo_StyleSet_ResolveForDeclarations, > - ServoComputedValuesStrong, > + ServoStyleContextStrong, > RawServoStyleSetBorrowed set, > - ServoComputedValuesBorrowedOrNull parent_style, > + ServoStyleContextBorrowedOrNull parent_style_context, Nit: This variable rename doesn't seem necessary. ::: layout/style/ServoBindingList.h:467 (Diff revision 5) > RawServoStyleSetBorrowed set) > -SERVO_BINDING_FUNC(Servo_ComputedValues_Inherit, ServoComputedValuesStrong, > +SERVO_BINDING_FUNC(Servo_ComputedValues_Inherit, ServoStyleContextStrong, > RawServoStyleSetBorrowed set, > - ServoComputedValuesBorrowedOrNull parent_style, > + mozilla::CSSPseudoElementType pseudo_type, > + nsIAtom* pseudo_tag, > + ServoStyleContextBorrowedOrNull parent_style_context, Same here.
Attachment #8884616 - Flags: review?(bobbyholley) → review+
Comment on attachment 8884615 [details] Bug 1367904 - Part 11: stylo: Use ServoStyleContext in ServoStyleSet/ServoRestyleManager; https://reviewboard.mozilla.org/r/155488/#review161014 ::: layout/style/ServoStyleContext.h:30 (Diff revision 4) > - Destructor(); > + // can't do off main thread > + // XXXManishearth fix this > + // CSSVariableImageTable::RemoveAll(this); See my comments on the previous patch regarding this, and update this appropriately.
Attachment #8884615 - Flags: review?(bobbyholley) → review+
Comment on attachment 8885444 [details] Bug 1367904 - Part 12: stylo: Fix assertions and behavior changes; https://reviewboard.mozilla.org/r/156298/#review161438 ::: layout/base/nsCSSFrameConstructor.cpp:5878 (Diff revision 1) > // styling descendants of elements with a -moz-binding the > // first time. Thus call StyleNewChildren() again. > styleSet->StyleNewChildren(element); > > styleContext = > - styleSet->ResolveStyleFor(element, nullptr, LazyComputeBehavior::Allow); > + styleSet->ResolveStyleFor(element, styleContext->GetParentAllowServo()->AsServo(), This should be LazyComputeBehavior::Assert, and always grab a style from the element. Otherwise the StyleNewChildren call wouldn't work, and everything would be crashing. ::: layout/style/ServoStyleContext.cpp:33 (Diff revision 1) > // No need to call ApplyStyleFixups here, since fixups are handled by Servo when > // producing the ServoComputedValues. > } > + > +void > +ServoStyleContext::ResolveSameStructsAs(nsPresContext* aPresContext, ServoStyleContext* aOther) `const ServoStyleContext* aOther`?
Comment on attachment 8885444 [details] Bug 1367904 - Part 12: stylo: Fix assertions and behavior changes; https://reviewboard.mozilla.org/r/156298/#review161446 r=me with my and emilio's comments addressed (in general I'm assuming you'll address emilio's comments on all these patches). ::: layout/style/ServoStyleContext.cpp:32 (Diff revision 1) > > // No need to call ApplyStyleFixups here, since fixups are handled by Servo when > // producing the ServoComputedValues. > } > + > +void Can you move this back to the header? I really want to make sure it gets inlined. I'd also accept evidence that this will definitely get inlined as-is.
Attachment #8885444 - Flags: review?(bobbyholley) → review+
Comment on attachment 8878669 [details] Bug 1367904 - Part 16: stylo: Stop using mStyleIfVisited in Servo; https://reviewboard.mozilla.org/r/149970/#review154694 > Is there any way to make this less fragile, or at least static-assert we do the right thing? (Disregard this if it's done in other patch) This will fail the layout tests if done wrong. > Why `ServoComputedValues2`? The name seems... fun. > > Also, this needs at least a general "why is this here" comment. (it's a temporary name because ServoComputedValues already means something and I don't want to confuse things further)
Comment on attachment 8878698 [details] Bug 1367904 - Part 4: stylo: Replace real ComputedValues with bindgenned ComputedValues2; https://reviewboard.mozilla.org/r/150006/#review159276 > I guess at this point we could drop the `gecko: Foo` from both sides. Is there any point on keeping it? Yes, but preferably as a followup. (I have a list of followups to work on or file ocne this lands)
Comment on attachment 8879798 [details] Bug 1367904 - Part 6: stylo: Introduce ComputedValuesInner; https://reviewboard.mozilla.org/r/151182/#review159280 > This needs docs. Why is it needed? I see nothing either here or in the commit message, and I find this super-confusing (and ugly). (transient, gets removed later)
Comment on attachment 8879799 [details] Bug 1367904 - Part 7: stylo: Use ComputedValuesInner instead of ComputedValues when we don't need it; https://reviewboard.mozilla.org/r/151184/#review156882 > Hm. This, on the other hand, seems much less optimal. Previously we always just shared the original allocation of the default values for placeholder frames (which pass null as the parent [1]) whereas now we'll heap-allocate a new one every time. > > However, it is true that, when we start allocating fused objects, we can't just blindly re-use the default ComputedValues, because placeholder frames are tagged with a specific anonymous box (oofPlaceholder). > > So how about the following: > (1) change the assertion from just asserting against text nodes to asserting that we're resolving for placeholder. > (2) Add a lazily-instantiated cached_placeholder_style: Option<Arc<ComputedValues>> alongside the default computed values. We should set it to None whenever the default computed values change. > (3) Use that cache here. > > Doing this as a separate patch (in the same bug) is fine/preferable. > > [1] http://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/layout/style/ServoStyleSet.cpp#538 [This will be done as a followup in the same bug, leaving open for now]
Comment on attachment 8884616 [details] Bug 1367904 - Part 10: stylo: Switch Gecko over to ServoStyleContext; https://reviewboard.mozilla.org/r/155490/#review160950 > Elaborate? It is no longer wrong (aside from the fact that GetContext should be removed eventually). Comment removed. > Nit: Space after the ?. But why do we need to null-check where we didn't before? Can't call `AsServo()` otherwise. > Were there any changes to this function? I can't tell. No. I do change the mStyleIfVisited stuff to `IsServo() || !mStyleIsVisited` in a later patch. > Why do we know this is an InheritingAnonBox? I thought it was only called once, but I was wrong. Fixed.
Comment on attachment 8886062 [details] Bug 1367904 - Part 13: stylo: Flatten ServoComputedValues into ServoStyleContext; https://reviewboard.mozilla.org/r/156858/#review162110 ::: layout/style/ServoBindings.cpp:223 (Diff revision 1) > +ServoComputedValues::ServoComputedValues(const ServoComputedValues* aValue) { > + PodAssign(this, aValue); > } Per IRC discussion, this constructor is too dangerous. Let's do the PodAssign in the ServoStyleContext ctor, and make that ctor take some newtype (like ServoComputedValuesForgotten) to make it clear that the memory has been forgotten by the caller.
Attachment #8886062 - Flags: review?(bobbyholley) → review-
Did the newtype/forget/ctor fix, and also got rid of the Servo_GetStyleFoo FFI call. Transitions are still not working (since Part 9), and Part 13 introduces a rare leak which I haven't figured out how to fix. The leak only happens on some tests and I can see which testsuites have it (https://treeherder.mozilla.org/#/jobs?repo=try&revision=6edf477940bcd347de71b4f225f60e5ab8d141f7) but not which test it is a result of. My current list of followups (not all in the same bug) are: - Remove GetContext(). Right now it doesn't impose additional overhead, but it shouldn't be necessary either - Figure out if we can do more sharing for visited styles. We create fresh visited styles very often. - Remove the whole `self.gecko` stuff from the servo-side reprs of the style structs - Stop passing around pseudoTag However, it basically works for most stuff, so it would be nice to get some perf testing to see how much of an improvement it is. Most of the followups won't impact perf, except for the visited style stuff, but I'm not going to fix that in this bug anyway.
Comment on attachment 8886062 [details] Bug 1367904 - Part 13: stylo: Flatten ServoComputedValues into ServoStyleContext; https://reviewboard.mozilla.org/r/156858/#review162668 ::: layout/style/ServoStyleContext.cpp:22 (Diff revision 2) > nsPresContext* aPresContext, > nsIAtom* aPseudoTag, > CSSPseudoElementType aPseudoType, > - already_AddRefed<ServoComputedValues> aComputedValues) > + const ServoComputedValues* aComputedValues) > : nsStyleContext(aParent, aPseudoTag, aPseudoType), > - mSource(Move(aComputedValues)) > + mSource(aComputedValues->Forget()) I don't think this makes sense. The "forgetting" already happened in Rust code. Basically, what I'd like to see is for Gecko_ServoStyleContext_Init (the FFI entry point) to explicitly convert its argument to a ServoComputedValuesForgotten, and then pass that down into the constructors. ::: layout/style/ServoTypes.h:173 (Diff revision 2) > } // namespace mozilla > > > +struct ServoComputedValues; > +struct ServoComputedValuesForgotten { > + ServoComputedValuesForgotten(const ServoComputedValues* aValue) : mPtr(aValue) {} Mark this |explicit|, and change the code appropriately in the caller. Making this constructor implicit undermines the whole reason for having it. ::: layout/style/ServoTypes.h:217 (Diff revision 2) > + // Make sure you manually mem::forget the backing ServoComputedValues > + // after calling this > + ServoComputedValuesForgotten Forget() const { > + return ServoComputedValuesForgotten(this); > + } Per the other comments, this should go away.
Attachment #8886062 - Flags: review?(bobbyholley) → review+
Comment on attachment 8886491 [details] Bug 1367904 - Part 14: stylo: Remove FFI calls for fetching style structs from ServoComputedValues; https://reviewboard.mozilla.org/r/157292/#review162670 ::: layout/style/ServoStyleContext.cpp:32 (Diff revision 1) > +void ServoStyleContext::ResolveSameStructsAs(nsPresContext* aPresContext, const ServoStyleContext* aOther) > +{ I think I previously asked this to remain inline?
Attachment #8886491 - Flags: review?(bobbyholley) → review+
Comment on attachment 8886491 [details] Bug 1367904 - Part 14: stylo: Remove FFI calls for fetching style structs from ServoComputedValues; https://reviewboard.mozilla.org/r/157292/#review162670 > I think I previously asked this to remain inline? It can't, because the `GetStyleFoo()` functions need a full definition of the style structs to exist. There's a cyclic dependency here :| Might be able to move them into a new inlines file, but I'd prefer to do the followup where I remove the whole `self.gecko` business which also lets us fix GetStyleFoo.
(In reply to Manish Goregaokar [:manishearth] from comment #164) > Comment on attachment 8886491 [details] > Bug 1367904 - Part 14: stylo: Remove FFI calls for fetching style structs > from ServoComputedValues; > > https://reviewboard.mozilla.org/r/157292/#review162670 > > > I think I previously asked this to remain inline? > > It can't, because the `GetStyleFoo()` functions need a full definition of > the style structs to exist. There's a cyclic dependency here :| You can just put it in a ServoStyleContextInlines.h.
Attachment #8885444 - Attachment is obsolete: true
Comment on attachment 8884616 [details] Bug 1367904 - Part 10: stylo: Switch Gecko over to ServoStyleContext; https://reviewboard.mozilla.org/r/155490/#review162700 ::: dom/animation/KeyframeEffectReadOnly.cpp:189 (Diff revision 9) > if (aRv.Failed()) { > return; > } > > RefPtr<nsStyleContext> styleContext = GetTargetStyleContext(); > - SetKeyframes(Move(keyframes), styleContext); > + SetKeyframes(Move(keyframes), styleContext->AsGecko()); This is an overkill. This code is used for both gecko and stylo. I guess we can leave this as it is if you leave SetKeyframes(nsTArray<Keyframe>&& aKeyframes, nsStyleContext* aStyleContext) and the static_assert() in DoSetKeyframes().
Yeah, I noticed it was asserting. I have a fix in https://hg.mozilla.org/try/rev/51710bd2441f31a7e526988bafef57040ec25f51
Review comments fixed, rebased again. We now crash on http://searchfox.org/mozilla-central/source/layout/base/ServoRestyleManager.cpp#101-102 for layout/reftests/css-display/display-contents-acid-dyn* , but emilio says that's a bug in the assertion there so I'm just going to ignore it for now.
Animation bug was because RawOffsetArc autoimplemented shallow equality, but nsTransitionManager::DoUpdateTransitions does a deep equality check to decide if the transition needs updating. So we were creating a transition and immediately popping it off the element because we thought it was stale.
The leak is in layout/style/crashtests/1200568-1.html (still works if you replace the animation with a simpler rule). Basically, if you create an element, (but don't bind to tree), and flush computed styles, it will leak the computed style (and perhaps the style context). Unsure why this is happening so far. The leak originates from Part 10.
The latest patch (Part 15) attempts to fix the leak. The leak no longer happens for 1200568-1.html but a try run is pending. Basically, nsComputedDOMStyle stores an ArenaRefPtr, and I think if that's used with ServoStyleContext (which is not arena allocated) there are issues. We already were storing SSCs in it so I'm not sure why it didn't break before -- Part 10 makes us hold on to SSCs for longer in certain cases and I suspect the order in which destruction happens affects if this bug can be observed. Unsure if Part 15 *is* the solution, so holding off on properly investigating and documenting it till try gets through. No need to review yet, but feel free to do so!
Okay, so it passes, but now animations_omta times out. https://treeherder.mozilla.org/#/jobs?repo=try&revision=be83ac30b7a2ac9ea7a9b54144ad133714071e37 It seems like this was introduced in Part 10 after a rebase. *sigh*
Comment on attachment 8886904 [details] Bug 1367904 - Part 15: stylo: Override ArenaRefPtr for ServoStyleContexts; https://reviewboard.mozilla.org/r/157628/#review162742 ::: layout/base/ArenaRefPtr.h:157 (Diff revision 1) > - if (mPtr && !sameArena) { > + if (mPtr && !sameArena && ArenaRefPtrTraits<T>::UsesArena(mPtr)) { > MOZ_ASSERT(mPtr->Arena()); > mPtr->Arena()->DeregisterArenaRefPtr(this); > } > mPtr = Move(aPtr); > - if (mPtr && !sameArena) { > + if (mPtr && !sameArena && ArenaRefPtrTraits<T>::UsesArena(aPtr)) { Nit: Rather than doing this, just assert that the value of UsesArena is the same for mPtr and aPtr, and then just do this check at the top of the function.
Attachment #8886904 - Flags: review?(bobbyholley) → review+
(In reply to Manish Goregaokar [:manishearth] from comment #201) > The latest patch (Part 15) attempts to fix the leak. The leak no longer > happens for 1200568-1.html but a try run is pending. > > Basically, nsComputedDOMStyle stores an ArenaRefPtr, and I think if that's > used with ServoStyleContext (which is not arena allocated) there are issues. > We already were storing SSCs in it so I'm not sure why it didn't break > before -- Part 10 makes us hold on to SSCs for longer in certain cases and I > suspect the order in which destruction happens affects if this bug can be > observed. > > Unsure if Part 15 *is* the solution, so holding off on properly > investigating and documenting it till try gets through. No need to review > yet, but feel free to do so! Why doesn't it leak now but leaks with your patches? We definitely store ServoStyleContexts in ArenaRefPtr, and not seem to leak. I'm afraid part 15 is only wallpapering some deeper issue.
Flags: needinfo?(manishearth)
The deeper issue is that due to the casts at http://searchfox.org/mozilla-central/rev/01d27fdd3946f7210da91b18fcccca01d7324fe2/layout/base/nsPresArena.cpp#45-70 we should *never* be sticking an nsStyleContext that is not Gecko-backed into the arena. We should MOZ_ASSERT(IsGecko()) in the arena id method on nsStyleContext, or expand ArenaRefPtrTraits. This bug is only observable if the arena is the thing to actually call the destructor, and these patches make us hold on to ServoStyleContext longer/shorter in some situations, which is probably why we didn't hit it so far.
Flags: needinfo?(manishearth)
The animations_omta issue is just that visited styles are totally broken (by part 13), and somehow we have only one test that fails when you break visited styling.
Attachment #8878670 - Attachment is obsolete: true
Attachment #8878671 - Attachment is obsolete: true
Attachment #8878698 - Attachment is obsolete: true
Attachment #8879797 - Attachment is obsolete: true
Attachment #8879798 - Attachment is obsolete: true
Attachment #8879799 - Attachment is obsolete: true
Attachment #8880262 - Attachment is obsolete: true
Attachment #8884616 - Attachment is obsolete: true
Attachment #8884615 - Attachment is obsolete: true
Fixed it. The problem was that we stopped doing the GetContext() call in ProcessPostTraversal; however this call is important for setting the NS_STYLE_RELEVANT_LINK_VISITED bit. I changed it so that we stop using mStyleIfVisited in Servo, and purely rely on the servo-side visited style (which we can now transparently fetch from gecko). Part 16 is partial, pushing it up to just make a note of progress (and to wait for try). I intend to move mStyleIfVisited over to GeckoComputedValues and rename ServoStyleSet::GetContext to something else (FinalizeContext or something) and have it modify the style context, not return it.
For some reason mozreview discarded the older patches?
(In reply to Manish Goregaokar [:manishearth] from comment #205) > The deeper issue is that due to the casts at > http://searchfox.org/mozilla-central/rev/ > 01d27fdd3946f7210da91b18fcccca01d7324fe2/layout/base/nsPresArena.cpp#45-70 > we should *never* be sticking an nsStyleContext that is not Gecko-backed > into the arena. Ah yes, that would explain it. > We should MOZ_ASSERT(IsGecko()) in the arena id method on nsStyleContext, or > expand ArenaRefPtrTraits. Yeah, for the scope of this bug, we should just disable the ArenaRefPtr machinery for ServoStyleContext. We can tune the rest in bug 1379830.
Comment on attachment 8878669 [details] Bug 1367904 - Part 16: stylo: Stop using mStyleIfVisited in Servo; https://reviewboard.mozilla.org/r/149970/#review163130
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #255) > Comment on attachment 8878669 [details] > Bug 1367904 - Part 16: stylo: Stop using mStyleIfVisited in Servo; > > https://reviewboard.mozilla.org/r/149970/#review163130 (because mozreview is broken): r=me on Part 16
Blocks: 1381635
Comment on attachment 8887172 [details] Bug 1367904 - Part 17: stylo: Clean up GetContext, call it UpdateWithElementState; https://reviewboard.mozilla.org/r/157914/#review163136
Attachment #8887172 - Flags: review?(bobbyholley) → review+
Comment on attachment 8887275 [details] Bug 1367904 - Part 18: stylo: Mark visited failures as expected; https://reviewboard.mozilla.org/r/158070/#review163256
Attachment #8887275 - Flags: review?(jryans) → review+
Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/550a24fe4812 Part 1: stylo: Create C++-side ServoComputedValues; r=bholley https://hg.mozilla.org/integration/autoland/rev/a8dd59eb6901 Part 3: stylo: Use RawOffsetArc in ComputedValues; r=bholley https://hg.mozilla.org/integration/autoland/rev/bea87ea9d86a Part 4: stylo: Replace real ComputedValues with bindgenned ComputedValues2; r=bholley https://hg.mozilla.org/integration/autoland/rev/7c1ac965de93 Part 5: stylo: Make GetBaseComputedValuesForElement return a style context; r=bholley https://hg.mozilla.org/integration/autoland/rev/055d3ed0202c Part 6: stylo: Introduce ComputedValuesInner; r=bholley https://hg.mozilla.org/integration/autoland/rev/914f8a42fa69 Part 10: stylo: Switch Gecko over to ServoStyleContext; r=bholley https://hg.mozilla.org/integration/autoland/rev/4d5e5d40c7ee Part 11: stylo: Use ServoStyleContext in ServoStyleSet/ServoRestyleManager; r=bholley https://hg.mozilla.org/integration/autoland/rev/c55df972f7c3 Part 13: stylo: Flatten ServoComputedValues into ServoStyleContext; r=bholley https://hg.mozilla.org/integration/autoland/rev/bbad2cf92d87 Part 14: stylo: Remove FFI calls for fetching style structs from ServoComputedValues; r=bholley https://hg.mozilla.org/integration/autoland/rev/25132d80a89e Part 15: stylo: Override ArenaRefPtr for ServoStyleContexts; r=bholley https://hg.mozilla.org/integration/autoland/rev/574fb214c95c Part 16: stylo: Stop using mStyleIfVisited in Servo; r=bholley https://hg.mozilla.org/integration/autoland/rev/a65aab119c7f Part 17: stylo: Clean up GetContext, call it UpdateWithElementState; r=bholley https://hg.mozilla.org/integration/autoland/rev/219ca1919e9b Part 18: stylo: Mark visited failures as expected; r=jryans
So this may be breaking the hazard build for some weird reason (normal builds work, but the hazard build can't find Servo_StyleContext_Release) Fix should be something like diff --git a/layout/style/ServoStyleContext.h b/layout/style/ServoStyleContext.h index 048e7a1..809605e 100644 --- a/layout/style/ServoStyleContext.h +++ b/layout/style/ServoStyleContext.h @@ -10,6 +10,15 @@ #include "nsStyleContext.h" namespace mozilla { + class ServoStyleContext; +} + +extern "C" { + void Servo_StyleContext_AddRef(const mozilla::ServoStyleContext* ctx); + void Servo_StyleContext_Release(const mozilla::ServoStyleContext* ctx); +} + +namespace mozilla { namespace dom { class Element;
Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/85928fde7206 Part 18: stylo: Move binding functions to ServoBindingList.h; r=bustage
(In reply to Pulsebot from comment #278) > Pushed by manishearth@gmail.com: > https://hg.mozilla.org/integration/autoland/rev/85928fde7206 > Part 18: stylo: Move binding functions to ServoBindingList.h; r=bustage What you really should do is, add a new line into ServoArcTypeList.h, and everything else would work as expected. Your current setting causes inconsistency on function type of Servo_StyleContext_{AddRef,Release}. In components/style/gecko/arc_types.rs, they have &ServoStyleContext, while in ServoBindingList.h you gave it pointer type. (This would cause test-stylo to fail if anyone tries to update bindings.rs.) If you add the type into ServoArcTypeList.h, it would automatically choose the correct signature, as well as bindgen options. That says, you shouldn't need to add the raw-lines, hide-types, or mapped-generic-types for ServoStyleContext at all. Could you fix that? (I'm just gonna sleep so wouldn't be able to land any code. I noticed this because hiro pinged me for the test-stylo failure he met.)
Flags: needinfo?(manishearth)
Thank you Xidorn! FWIW, the failure is; error[E0308]: mismatched types --> /home/travis/build/servo/servo/target/geckolib/debug/build/stylo_tests-3388546261329d90/out/check_bindings.rs:55:34 | 55 | [ Servo_StyleContext_AddRef, bindings::Servo_StyleContext_AddRef ]; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected reference, found *-ptr | = note: expected type `unsafe extern "C" fn(&style::gecko_bindings::structs::ServoStyleContext) {style::gecko::arc_types::Servo_StyleContext_AddRef}` found type `unsafe extern "C" fn(*mut style::gecko_bindings::structs::ServoStyleContext) {style::gecko_bindings::bindings::Servo_StyleContext_AddRef}` error[E0308]: mismatched types --> /home/travis/build/servo/servo/target/geckolib/debug/build/stylo_tests-3388546261329d90/out/check_bindings.rs:56:35 | 56 | [ Servo_StyleContext_Release, bindings::Servo_StyleContext_Release ]; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected reference, found *-ptr
ServoArcTypeList won't work because ServoStyleContext is namespaced and the macros can't deal with that. Pushed a fix, and will work towards making the macros namespaced.
Flags: needinfo?(manishearth)
Depends on: 1381895
Blocks: 1382017
Attachment #8887038 - Flags: review?(bobbyholley) → review+
Attachment #8887039 - Flags: review?(bobbyholley) → review+
Attachment #8887040 - Flags: review?(bobbyholley) → review+
Attachment #8887041 - Flags: review?(bobbyholley) → review+
Attachment #8887042 - Flags: review?(bobbyholley) → review+
Attachment #8887043 - Flags: review?(bobbyholley) → review+
Attachment #8887044 - Flags: review?(bobbyholley) → review+
Attachment #8887045 - Flags: review?(bobbyholley) → review+
Attachment #8887046 - Flags: review?(bobbyholley) → review+
Attachment #8887047 - Flags: review?(bobbyholley) → review+
(the mozreview screwiness left these review requests open).
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: