Closed Bug 1466963 Opened 6 years ago Closed 6 years ago

DeclarationBlock should provide some sort of before-mutation callback to C++

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(9 files)

Bug 1466963: Inline some trivial bits.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1466963: Fix a typo.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1466963: Remove unused PropertyDeclarationBlock::set_importance.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1466963: Add a before-change callback to remove_property.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1466963: Trivially simplify a condition.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1466963: Use consistent indentation in DeclarationBlock, and make the copy-constructor private.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1466963: Inline DeclarationBlock's methods since they just forward to Servo.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1466963: Provide a before-mutation closure to C++.
59 bytes, text/x-review-board-request
smaug
: review+
xidorn
: review+
Details
followup
3.43 KB, patch
emilio
: review+
Details | Diff | Splinter Review
In order to avoid unconditionally cloning in bug 1428246.
Comment on attachment 8983580 [details] Bug 1466963: Inline some trivial bits. https://reviewboard.mozilla.org/r/249444/#review255696 TBH I'm not convenced that this patch is useful. The compiler should be able to inline anything within a crate. I doubt how useful is this patch. If it turns out that the compiler cannot inline these functions, that should be considered a bug in the compiler.
Attachment #8983580 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8983581 [details] Bug 1466963: Fix a typo. https://reviewboard.mozilla.org/r/249446/#review255702 ::: servo/components/style/properties/declaration_block.rs:485 (Diff revision 1) > } > } > changed > } > > /// Adds or overrides the declaration for a given property in this block. Maybe the document should mention the meaning of the returned value.
Attachment #8983581 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8983582 [details] Bug 1466963: Remove unused PropertyDeclarationBlock::set_importance. https://reviewboard.mozilla.org/r/249448/#review255704
Attachment #8983582 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8983584 [details] Bug 1466963: Trivially simplify a condition. https://reviewboard.mozilla.org/r/249452/#review255706
Attachment #8983584 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8983586 [details] Bug 1466963: Inline DeclarationBlock's methods since they just forward to Servo. https://reviewboard.mozilla.org/r/249456/#review255712 I'm in general not very happy with adding dependencies to `ServoBindings.h` from header files, but it seems we are not including DeclarationBlock.h in any other header file so it's probably fine.
Attachment #8983586 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8983587 [details] Bug 1466963: Provide a before-mutation closure to C++. https://reviewboard.mozilla.org/r/249458/#review255792
Attachment #8983587 - Flags: review?(bugs) → review+
Comment on attachment 8983585 [details] Bug 1466963: Use consistent indentation in DeclarationBlock, and make the copy-constructor private. https://reviewboard.mozilla.org/r/249454/#review255708 ::: layout/style/DeclarationBlock.h:62 (Diff revision 1) > - bool IsMutable() const { > + bool IsMutable() const > + { > return !mImmutable; > } This can be in one line.
Attachment #8983585 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8983587 [details] Bug 1466963: Provide a before-mutation closure to C++. https://reviewboard.mozilla.org/r/249458/#review256056 It seems you are adding the callback stuff without actually making use of it here? I'd like to see it's actual usage before reviewing this (as well as part 1).
Attachment #8983587 - Flags: review?(xidorn+moz)
Comment on attachment 8983587 [details] Bug 1466963: Provide a before-mutation closure to C++. https://reviewboard.mozilla.org/r/249458/#review256056 I mean, part 4... The "Add a before-change callback to remove_property" part.
Comment on attachment 8983583 [details] Bug 1466963: Add a before-change callback to remove_property. https://reviewboard.mozilla.org/r/249450/#review256064 As I mentioned in comment 16, I would like to see how we are actually going to use it first. ::: servo/components/style/properties/declaration_block.rs:598 (Diff revision 1) > + } > + > + if !removed_at_least_one { > + before_change_callback(&*self); > + } > removed_at_least_one = true; This can be moved into the if-block above, as you don't need to set it again when it's already set. ::: servo/components/style/properties/declaration_block.rs:599 (Diff revision 1) > if let PropertyDeclarationId::Longhand(id) = id { > - longhands.remove(id); > + self.longhands.remove(id); > } > - declarations_importance.remove(i); > + self.declarations_importance.remove(i); I would probably prefer we make the mutation part together (out of the scope). But it's probably not a big deal, and when NLL comes we should be able to remove the extra scope.
Attachment #8983583 - Flags: review?(xidorn+moz)
I'm going to implement the stuff needing the callback. Basically make bug 1428246 to work in such way that we don't need to do any cloning for MutationObserver nor attributeChanged callback. (cloning declaration is very slow)
And I'm hoping this lands before I work on that stuff. But basically styling code needs to notify DOM before any changes, but not notify if anything isn't about to change. And DOM needs to be notified also after the change.
So I suppose that what you are going to do is to have the callback grab the serialization of the declaration block just before we are trying to actually mutate it, so that we don't need to keep the old declaration block around just for getting the old value, is that correct? In addition to that, I suppose given that it gives us the precise call point before mutation to the declaration block, it would also enable us to fix bug 1197705. There is one thing which is still unclear to me, though, that we are currently cloning declaration block when the block is not dirty[1], which seems to be triggered much more common than the immutability condition, and I don't see how the new callback here is going to address that clone. If we are not addressing that clone, the only clone where this new callback can remove is, when a declaration block is updated multiple times between two restyles. That may be reasonable common as people tend to update several different properties in a row. But in majority of those cases, I suspect we should be able to collapse those changes into a single record / event, and thus shouldn't need to collect old value for each access. That collapsing should be doable in a different way without much problem. Based on these, I'm not totally convinced that this is going to help performance much. We may need something like this for conformance / correctness, though. [1] https://searchfox.org/mozilla-central/rev/3737701cfab93ccea04c0e9cab211ad10f931d87/layout/style/DeclarationBlock.h#100-107
Based on our code coverage data, we are triggering clone for not being dirty far more usual than for being immutable (>10k vs. ~700), but that's probably because our tests are generally eager to trigger restyle than normal content document.
The patch in bug 1428246 gives correctness. But the forced SetImmutable call regresses performance badly when one has MutationObservers.
As I mentioned in comment 21, I believe it is possible to track whether one has had mutation record and avoid setting immutable in that case. But maybe it is not that easy? I suppose even with the callback available, you may want to avoid serializing the declaration block repeatedly when doing several style.foo calls in a row either.
I don't understand that comment. each style.foo = "bar" should create a new MutationRecord (assuming .foo is a valid property name, and "bar" valid value). And serializing should happen only if MutationObserver observers attributeOldValue
Oh, okay, I thought we only generate one record for consecutive settings, but we actually need to generate one for each setting. That's a bit surprised, but... okay, I'll have another look at the patches with this usage in mind.
Comment on attachment 8983587 [details] Bug 1466963: Provide a before-mutation closure to C++. https://reviewboard.mozilla.org/r/249458/#review256078 ::: layout/style/ServoBindings.toml:213 (Diff revision 1) > "nsContentUtils_.*", > "GECKO_IS_NIGHTLY", > ] > whitelist-types = [ > "RawGecko.*", > + "DeclarationBlockMutationClosure", I would probably prefer if you can blacklist it and add it as an alias, which can avoid the pointer casting, and thus ensure every appearance of this type in Rust is representing the same thing. ::: servo/ports/geckolib/glue.rs:178 (Diff revision 1) > +impl ClosureHelper for DeclarationBlockMutationClosure { > + #[inline] > + fn invoke(&self, decls: &PropertyDeclarationBlock) { > + if let Some(function) = self.function.as_ref() { > + unsafe { > + function(decls as *const _ as *const _, self.data); If you alias `DeclarationBlockMutationClosure` to `PropertyDeclarationBlock`, you should be able to remove the pointer castings here. ::: servo/ports/geckolib/glue.rs:3417 (Diff revision 1) > -pub extern "C" fn Servo_DeclarationBlock_GetCssText(declarations: RawServoDeclarationBlockBorrowed, > - result: *mut nsAString) { > +pub unsafe extern "C" fn Servo_DeclarationBlock_GetCssText( > + declarations: RawServoDeclarationBlockBorrowed, > + result: *mut nsAString, > +) { > read_locked_arc(declarations, |decls: &PropertyDeclarationBlock| { > - decls.to_css(unsafe { result.as_mut().unwrap() }).unwrap(); > + decls.to_css(&mut *result).unwrap() I would prefer `.as_mut().unwrap()`, which is sound when someone passes nullptr in. ::: servo/ports/geckolib/glue.rs:3426 (Diff revision 1) > #[no_mangle] > +pub unsafe extern "C" fn Servo_UnlockedDeclarationBlock_GetCssText( > + declarations: *const structs::RawServoUnlockedDeclarationBlock, > + result: *mut nsAString, > +) { > + let decls = &*(declarations as *const PropertyDeclarationBlock); As mentioned above, if we can have the binding type aliased, you wouldn't need this cast. A `.as_ref().unwrap()` would be better. ::: servo/ports/geckolib/glue.rs:3580 (Diff revision 1) > + return false; > + } > + > - let importance = if is_important { Importance::Important } else { Importance::Normal }; > + let importance = if is_important { Importance::Important } else { Importance::Normal }; > - write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| { > + write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| { > + before_change_closure.invoke(&*decls); `PropertyDeclarationBlock::extend` isn't guaranteed to return true, although it is currently. Maybe not a big issue either way as far as we don't trigger anything for invalid value...
Attachment #8983587 - Flags: review+
Comment on attachment 8983583 [details] Bug 1466963: Add a before-change callback to remove_property. https://reviewboard.mozilla.org/r/249450/#review256080
Attachment #8983583 - Flags: review+
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #28) > Comment on attachment 8983587 [details] > Bug 1466963: Provide a before-mutation closure to C++. > > https://reviewboard.mozilla.org/r/249458/#review256078 > > ::: layout/style/ServoBindings.toml:213 > (Diff revision 1) > > "nsContentUtils_.*", > > "GECKO_IS_NIGHTLY", > > ] > > whitelist-types = [ > > "RawGecko.*", > > + "DeclarationBlockMutationClosure", > > I would probably prefer if you can blacklist it and add it as an alias, > which can avoid the pointer casting, and thus ensure every appearance of > this type in Rust is representing the same thing. I'd need to use the same trick we use for nsString and such, and I don't think it's worth the complexity. > ::: servo/ports/geckolib/glue.rs:178 > (Diff revision 1) > > +impl ClosureHelper for DeclarationBlockMutationClosure { > > + #[inline] > > + fn invoke(&self, decls: &PropertyDeclarationBlock) { > > + if let Some(function) = self.function.as_ref() { > > + unsafe { > > + function(decls as *const _ as *const _, self.data); > > If you alias `DeclarationBlockMutationClosure` to > `PropertyDeclarationBlock`, you should be able to remove the pointer > castings here. > > ::: servo/ports/geckolib/glue.rs:3417 > (Diff revision 1) > > -pub extern "C" fn Servo_DeclarationBlock_GetCssText(declarations: RawServoDeclarationBlockBorrowed, > > - result: *mut nsAString) { > > +pub unsafe extern "C" fn Servo_DeclarationBlock_GetCssText( > > + declarations: RawServoDeclarationBlockBorrowed, > > + result: *mut nsAString, > > +) { > > read_locked_arc(declarations, |decls: &PropertyDeclarationBlock| { > > - decls.to_css(unsafe { result.as_mut().unwrap() }).unwrap(); > > + decls.to_css(&mut *result).unwrap() > > I would prefer `.as_mut().unwrap()`, which is sound when someone passes > nullptr in. I don't think I'm really worried about passing nulls to these functions. We use panic=abort anyway, it only provides a "called unwrap on a None value" instead of a null crash, which I'm not sure is that useful. > ::: servo/ports/geckolib/glue.rs:3426 > (Diff revision 1) > > #[no_mangle] > > +pub unsafe extern "C" fn Servo_UnlockedDeclarationBlock_GetCssText( > > + declarations: *const structs::RawServoUnlockedDeclarationBlock, > > + result: *mut nsAString, > > +) { > > + let decls = &*(declarations as *const PropertyDeclarationBlock); > > As mentioned above, if we can have the binding type aliased, you wouldn't > need this cast. A `.as_ref().unwrap()` would be better. > > ::: servo/ports/geckolib/glue.rs:3580 > (Diff revision 1) > > + return false; > > + } > > + > > - let importance = if is_important { Importance::Important } else { Importance::Normal }; > > + let importance = if is_important { Importance::Important } else { Importance::Normal }; > > - write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| { > > + write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| { > > + before_change_closure.invoke(&*decls); > > `PropertyDeclarationBlock::extend` isn't guaranteed to return true, although > it is currently. Maybe not a big issue either way as far as we don't trigger > anything for invalid value... Yeah, I think we do want to trigger the mutation as long as the value is not invalid. If we decide we don't we can push this a bit further down but I don't think it's worth it right now.
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0488c32bbcdf Inline some trivial bits. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/dac3de037136 Fix a typo. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/43cf976e4f54 Remove unused PropertyDeclarationBlock::set_importance. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/3e80f0aa86c5 Add a before-change callback to remove_property. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e27ee3035e Trivially simplify a condition. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/6245a29f2539 Use consistent indentation in DeclarationBlock, and make the copy-constructor private. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/9463a7242770 Inline DeclarationBlock's methods since they just forward to Servo. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/a1ddb11380aa Provide a before-mutation closure to C++. r=xidorn,smaug
Attached patch followup — — Splinter Review
Attachment #8984353 - Flags: review?(emilio)
Comment on attachment 8984353 [details] [diff] [review] followup Review of attachment 8984353 [details] [diff] [review]: ----------------------------------------------------------------- Err, I totally forgot about the mapped-generic-types thingie. Thanks!
Attachment #8984353 - Flags: review?(emilio) → review+
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/69d18e5bb7dc followup - Make RawServoUnlockedDeclarationBlock an alias to PropertyDeclarationBlock in Servo side. r=emilio
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: