554687 - Create method nsSMILValue::Swap()

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

There are a number of places where we build up a temporary nsSMILValue, and if it succeeds, then we copy its value into a nsSMILValue outparam.

In cases where our nsSMILValue has extra memory allocated (using mU.mPtr), this requires us to copy all of that extra memory.  This is inefficient -- in general, we can just copy the pointer, if we clean up after ourselves.

We could handle this cleanly by adding a method "nsISMILType::TransferValue", which would be just like "Assign", except it would promise to leave the source null-typed. (and hence could just directly transfer the pointer, rather than needing to copy the memory it points to)

Places where this would be useful:
 - nsSMILAnimationFunction::ComposeResult
 - ValueFromString in jwatt's patch for Bug 515116
 - Probably other ValueFromString implementations

Comment 1

[Daniel Holbert [:dholbert]]

Alternately, this could probably just be a "Swap()" method. (transferring in both directions).

Usually we'll probably only care about one of the two directions, but Swap has the benefit of being a well-understood action (from a caller's perspective).

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: patch 1: Add nsSMILValue::Swap

This patch adds the method "Swap()", which swaps the complete contents of two nsSMILValue pointers.

This is done by just simple byte-copying (by copying the PRUint64 union-member).

(This could theoretically be a problem if we had nsSMILValue.mU.mPtr pointing to a struct that pointed back to the nsSMILValue. I don't think that's worth worrying about, though -- I think that sort of situation would be unnecessary and worth avoiding anyway.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: patch 2: use nsSMILValue::Swap where appropriate [ignore]

This adds uses of nsSMILValue::Swap in nsSMILAnimationFunction::ComposeResult and in various ValueFromString implementations where we populate our outparam from a temporary nsSMILValue variable.

This also removes two instances of OOM-checking in contextual code that are no longer needed after bug 550593.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 434639 [details] [diff] [review]
patch 2: use nsSMILValue::Swap where appropriate [ignore]

better version of patch 2 coming in a sec

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: patch 2: use nsSMILValue::Swap where appropriate

Arright, here's patch 2. Fixes w.r.t. previously-posted version:
 - Removed an unnecessary clearing of a nsSMILValue, in nsSMILAnimationFunction::ComposeResult
 - One of the OOM-check-removals was broken (in SMILViewBox::GetBaseValue()) -- corrected in this version. (caught it due to a failing reftest)

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In part 1, why not just implement it directly with four assignments instead of going through TransferValue?

Comment 7

[Daniel Holbert [:dholbert]]

Well, 6 assignments (cache my type & value in a temp var, overwrite my type & value based on aOther, overwrite aOther's type & value based on temp var).

That's fine, too -- I used the helper function TransferValue() to more clearly match the traditional 3-step Swap() function (using only 3 lines).

How about I make TransferValue inline, and only do the paranoia bit at the end in debug builds?

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: patch 1 v2: Add nsSMILValue::Swap

Ok, this patch simplifies things a bit:
 - The helper function is inline
 - The helper function leaves aSrc alone now (and changed name to reflect that)
 - Swap() tweaks tmp.mType at the very end, so Destroy() won't get invoked. (previously this was handled by the cleanup of aSrc in the helper function)

With the inlining, I think this expands to what you were looking for in comment 6, roc -- correct me if I'm wrong, though.

Comment 9

[Jonathan Watt [:jwatt]]

You may also want to PR_STATIC_ASSERT(sizeof(mU) == sizeof(PRUint64)) just to make sure we notice if we add a bigger member at some point in the future.

Comment 10

[Daniel Holbert [:dholbert]]

I think the existing PR_STATIC_ASSERT already takes care of that, by checking sizeof(PRUint64) + sizeof(mType pointer) = sizeof(nsSMILValue)

Comment 11

[Jonathan Watt [:jwatt]]

Right, sorry.

Comment 12

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=a4178a154d16
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=1bcf725bb32f

Comment 13

[Daniel Holbert [:dholbert]]

backed out due to PR_STATIC_ASSERT failure on ppc & windows (so far):
  http://hg.mozilla.org/mozilla-central/rev/c0a990361450
  http://hg.mozilla.org/mozilla-central/rev/267fd5608fe3

Not sure why sizeof(nsSMILValue) is larger than 64 + 32 on those platforms... maybe they round the size up to 128 to make byte-alignment nicer?  Anyway, I'll do a few diagnostics on try-server to see what's going on.  Worst case, we can just use memcpy.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess the size is rounded up so that alignment works, i.e. an array of nsSMILValues has each one aligned to an 8-byte boundary.

Comment 15

[Daniel Holbert [:dholbert]]

I suspect it's rather a 4-byte-boundary-alignment issue, with the short values "mUnit" & "mOrientType" that make up the struct "mOrient".
78   union {
...
83     struct {
84       float mAngle;             // 32 bits
85       PRUint16 mUnit;           // 16 bits (or 32 bits if word-aligned)
86       PRUint16 mOrientType;     // 16 bits (or 32 bits if word-aligned)
87     } mOrient;
...
89   } mU;

If those values are word-aligned, then we add 16 bits of wasted space for each of them, which makes the union larger than we'd otherwise expect.

I'm testing this theory with some Try Server diagnostics right now.  If mOrient is the culprit, then that means Bug 545042 accidentally increased the size of all nsSMILValue objects, and we should fix that. (perhaps by combining mUnit & mOrientType into a single 32-bit int? Or by declaring them with ":16;" to indicate exactly how many bits we want to allocate for them? (I'm not sure if that would work any better))

Comment 16

[Daniel Holbert [:dholbert]]

Nope, comment 15 is wrong -- nsSMILValue is 16 bytes on Windows even when I revert to pre-mOrient (parent changeset of Bug 545042's fix).

So, this is something like what roc suggests in comment 14.  I guess we need to just memcpy the whole nsSMILValue.

Comment 17

[Robert Longson [:longsonr]]

Or have an additional 4 byte padding member at the end and assert that size = sizeof int64 + sizeof type + sizeof padding.

Comment 18

[Daniel Holbert [:dholbert]]

Well, on Linux x86, any added padding would increase the size of our nsSMILValue structures (since right now, Linux is perfectly happy having them be 12 bytes)  And on 64-bit systems, mPtr will be 8 bytes instead of 4, so we also wouldn't want to add any padding on 64-bit systems.

So if we added any explicit padding, we'd probably need to wrap its declaration & the sizeof assertion in a somewhat complex & hacky #ifdef.  I think I prefer just memcpy'ing -- it's not really any more hacky than the currently-posted patch's strategy of directly assigning mU.mUint.

New patch with memcpy coming up in a sec.

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: patch 1 v3: Add nsSMILValue::Swap

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 435041 [details] [diff] [review]
patch 1 v3: Add nsSMILValue::Swap

New patch just replaces each CopyMemberData() call with an equivalent memcpy call. (I tweaked the header comment for Swap(), too.)

Comment 21

[Daniel Holbert [:dholbert]]

Re-landed:
 http://hg.mozilla.org/mozilla-central/rev/62ea9c99b289
 http://hg.mozilla.org/mozilla-central/rev/6eaaeb9c35e2