690335 - Bug 675553 may have increased the size of various SVG classes

Status: RESOLVED WORKSFORME

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

In bug 675553, PRPackedBool was replaced with bool. I believe this may have increased the size of various SVG classes because compilers don't tend to pack adjacent members if they are of different types. We should investigate if there are any increases on the various platforms, but I don't have time right now.

The changes from that bug to SVG code can be found using:

  hg diff -c e7854b4d29ba content/svg layout/svg

Comment 1

[Michael Wu [:mwu]]

FWIW, bool is 8 bits on every platform we support so I can't imagine it being any worse than PRPackedBool. But, it's worth checking anyway. It sounds like someone is planning to run some dehydra script to check data structure sizes after the switch.

Comment 2

[Jonathan Watt [:jwatt]]

It's not the size that's of concern, but the type of adjacent members. See bug 522306 comment 15 for recent (<1 year ago) confirmation that this is still the case.

Comment 3

[Daniel Holbert [:dholbert]]

I just pushed 2 builds to try, checking sizeof() with 3 SVG classes that formerly had PRPackedBool adjacent to PRUintXXX member-variables  (but now have bool there instead):
 DOMSVGTransform
 SVGPreserveAspectRatio
 SVGAnimatedPreserveAspectRatio

I'm just running SVG reftests, with printf of "sizeof(_class_): XX" inserted for these classes.  Based on bug 522306 comment 15, packing issues are most likely to crop up with MSVC / Windows.

Push with PRPackedBool:
https://tbpl.mozilla.org/?tree=Try&rev=44a1361fe41f
Push with bool:
https://tbpl.mozilla.org/?tree=Try&rev=3fd06cccaabf

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> I just pushed 2 builds to try, checking sizeof() with 3 SVG classes that
> formerly had PRPackedBool adjacent to PRUintXXX member-variables  (but now
> have bool there instead):
>  DOMSVGTransform

Er, s/DOMSVGTransform/SVGAnimatedLengthList/
(My patch did add a printf for DOMSVGTransform, too, but SVG reftests don't tickle the code with that printf)

So -- for the classes that I tested, the size remained the same before & after Bug 675553's bool switchcover.

32-bit Linux, Mac, Windows (opt and debug):
sizeof(SVGAnimatedLengthList): 8
sizeof(SVGAnimatedPreserveAspectRatio): 8
sizeof(SVGPreserveAspectRatio): 3

64-bit Linux, Mac (opt and debug):
sizeof(SVGAnimatedLengthList): 16
sizeof(SVGAnimatedPreserveAspectRatio): 8
sizeof(SVGPreserveAspectRatio): 3

64-bit Windows isn't done yet (and one of my Try pushes didn't seem to get a Win64 run at all for some reason), but I'm not super-concerned about Win64 since we don't support it yet (and I strongly suspect it'll be fine, based on the results above)

Comment 5

[Daniel Holbert [:dholbert]]

So, the SVGPreserveAspectRatio results show that we do successfully pack "PRUint8,PRUint8,bool" into 3 bytes.

We *could* potentially get issues from packing e.g.
 PRUint32:31,bool:1
into 4 bytes.  However, bug 522306 comment 15 showed that this was *already* a problem for PRPackedBool:1.  (that's why we stuck with PRUint32:1 instead of PRPackedBool:1 in that case.)

So Bug 675553 isn't responsible for trickering any size-increase from that.

It could feasibly be responsible for a size increase, *if* we had code like:
  PRUint8:7
  PRPackedBool:1
since PRPackedBool and PRUint8 are the same type under-the-hood, which would let them pack into a single byte.  (and swapping in a bool:1 would break that)

However, we _don't_ have any code like that (with PRUint8:whatever), so that's not a problem.

So I think we're OK.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> So Bug 675553 isn't responsible for trickering any size-increase from that.

er, "triggering" :)

> However, we _don't_ have any code like that (with PRUint8:whatever), so
> that's not a problem.

(at least, the "hg diff" command from comment 0 doesn't show any such code.)

Resolving this bug as WORKSFORME, since AFAICT there's no problem.