1422641 - stylo: Gecko_GetSMILOverrideDeclarationBlock appears on stylo-chrome profiles.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

And it's another unnecessary double-function-call we call all the time during cascading.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

This one doesn't even have a bit or anything guarding it...

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1422641: Inline Gecko_GetSMILOverrideDeclarationBlock.

Review commit: https://reviewboard.mozilla.org/r/204956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204956/

Comment 3

[Manish Goregaokar [:manishearth]]

Comment on attachment 8934025 [details]
Bug 1422641: Inline Gecko_GetSMILOverrideDeclarationBlock.

https://reviewboard.mozilla.org/r/204956/#review210400

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8934025 [details]
Bug 1422641: Inline Gecko_GetSMILOverrideDeclarationBlock.

https://reviewboard.mozilla.org/r/204956/#review210608

I'm fine with the Gecko side code removal, but there are some comments for Servo side change.

TBH I'm not totally happy with this kind of inlining, especially those duplicating code from Gecko :/

::: servo/components/style/gecko/wrapper.rs:1068
(Diff revision 1)
> -        let declarations: Option<&RawOffsetArc<Locked<PropertyDeclarationBlock>>> =
> -            declarations.and_then(|s| s.as_arc_opt());
> +            let declaration: &structs::DeclarationBlock =
> +                self.get_extended_slots()?.mSMILOverrideStyleDeclaration.mRawPtr.as_ref()?;

You cannot use `?` with `Option` at the moment, we haven't started requiring Rust 1.22. See bug 1421097.

::: servo/components/style/gecko/wrapper.rs:1072
(Diff revision 1)
> +            let declaration: &structs::ServoDeclarationBlock =
> +                mem::transmute(declaration);

Is there any approach we can static assert that `DeclarationBlock` is indeed aligned with `ServoDeclarationBlock`?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC-6 (less responsive Nov 5 ~ Dec 16) from comment #4)
> Comment on attachment 8934025 [details]
> Bug 1422641: Inline Gecko_GetSMILOverrideDeclarationBlock.
> 
> https://reviewboard.mozilla.org/r/204956/#review210608
> 
> I'm fine with the Gecko side code removal, but there are some comments for
> Servo side change.
> 
> TBH I'm not totally happy with this kind of inlining, especially those
> duplicating code from Gecko :/

Yeah, some of it is not great, but it does show up in profiles, and we have done this before for quite a while... It'd be nice to get cross-language LTO, but building firefox with clang-cl will take a while.

> ::: servo/components/style/gecko/wrapper.rs:1068
> (Diff revision 1)
> > -        let declarations: Option<&RawOffsetArc<Locked<PropertyDeclarationBlock>>> =
> > -            declarations.and_then(|s| s.as_arc_opt());
> > +            let declaration: &structs::DeclarationBlock =
> > +                self.get_extended_slots()?.mSMILOverrideStyleDeclaration.mRawPtr.as_ref()?;
> 
> You cannot use `?` with `Option` at the moment, we haven't started requiring
> Rust 1.22. See bug 1421097.

Hmm, this was green on CI though, weird...

> 
> ::: servo/components/style/gecko/wrapper.rs:1072
> (Diff revision 1)
> > +            let declaration: &structs::ServoDeclarationBlock =
> > +                mem::transmute(declaration);
> 
> Is there any approach we can static assert that `DeclarationBlock` is indeed
> aligned with `ServoDeclarationBlock`?

I can't think of an easy way off-hand, but I can try...

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06852d04e070
Inline Gecko_GetSMILOverrideDeclarationBlock. r=Manishearth,xidorn

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/06852d04e070