Closed Bug 1400100 Opened 2 years ago Closed 2 years ago

Shrink css::ImageValue

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(2 files)

This reduces sizeof(ImageValue) from 104 to 96. When heap-allocated, this moves
it from the 112 byte bin to the 96 byte bin. Loading gmail with Stylo, there
are about 11,500 ImageValues on the heap, so this saves about 184,000 bytes.
Attachment #8908463 - Flags: review?(cam)
Attachment #8908463 - Flags: review?(cam) → review+
Priority: -- → P3
Comment on attachment 8909159 [details]
Bug 1400100 - Shrink css::ImageValue. .

https://reviewboard.mozilla.org/r/180736/#review185790

heycam already r+'d this via Splinter review. I'm publishing it to mozreview so I can land it on autoland.
Attachment #8909159 - Flags: review+
Attachment #8909159 - Flags: review?(cam) → review+
https://hg.mozilla.org/mozilla-central/rev/7ca486b28b99
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8909159 [details]
Bug 1400100 - Shrink css::ImageValue. .

https://reviewboard.mozilla.org/r/180736/#review185920

::: commit-message-ae398:3
(Diff revision 1)
> +This reduces sizeof(ImageValue) from 104 to 96. When heap-allocated, this moves
> +it from the 112 byte bin to the 96 byte bin. Loading gmail with Stylo, there
> +are about 11,500 ImageValues on the heap, so this saves about 184,000 bytes.

I don't think it's going to work on Windows... but wouldn't be worse anyway.
> I don't think it's going to work on Windows...

Why not?
Flags: needinfo?(xidorn+moz)
Because IIRC MSVC doesn't pack fields across boundary of type, so the bool which moved upwards should still take a whole word because of the alignment of the next field. See bug 1363375 comment 9 for similar attempt.

Actually, it reminds me that it could make things worse, because Rust cannot do that either, and consequently it would break code generated by bindgen. froydnj may have one more trouble to deal with for bug 1373878 then.
Flags: needinfo?(xidorn+moz)
That makes me think we should actually backout this patch, otherwise when people try to update Servo's in-tree binding files, they may get into trouble.
You can compare the result of https://godbolt.org/g/z2PrQW with different compilers (x86-64/x86 CL in the drop-down list is MSVC, and you can also select Clang and specify "--target=x86_64-pc-windows-msvc").
Oh, and also you can see rust-lang-nursery/rust-bindgen#380 for this issue with bindgen.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #9)
> That makes me think we should actually backout this patch, otherwise when
> people try to update Servo's in-tree binding files, they may get into
> trouble.

Bug 1403073 works around this so no need to back out.
You need to log in before you can comment on or make changes to this bug.