Closed Bug 554687 Opened 10 years ago Closed 10 years ago

Create method nsSMILValue::Swap()


(Core :: SVG, defect)

Not set





(Reporter: dholbert, Assigned: dholbert)



(2 files, 3 obsolete files)

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
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).
Summary: Create method nsISMILType::TransferValue → Create method nsISMILType::TransferValue() (or Swap()?)
Attached patch patch 1: Add nsSMILValue::Swap (obsolete) — Splinter Review
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.)
Attachment #434633 - Flags: review?(roc)
Summary: Create method nsISMILType::TransferValue() (or Swap()?) → Create method nsSMILValue::Swap()
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 on attachment 434639 [details] [diff] [review]
patch 2: use nsSMILValue::Swap where appropriate [ignore]

better version of patch 2 coming in a sec
Attachment #434639 - Attachment description: patch 2: use nsSMILValue::Swap where appropriate → patch 2: use nsSMILValue::Swap where appropriate [ignore]
Attachment #434639 - Attachment is obsolete: true
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)
Attachment #434641 - Flags: review?(roc)
In part 1, why not just implement it directly with four assignments instead of going through TransferValue?
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?
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.
Attachment #434633 - Attachment is obsolete: true
Attachment #434664 - Flags: review?(roc)
Attachment #434633 - Flags: review?(roc)
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.
I think the existing PR_STATIC_ASSERT already takes care of that, by checking sizeof(PRUint64) + sizeof(mType pointer) = sizeof(nsSMILValue)
Right, sorry.
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
backed out due to PR_STATIC_ASSERT failure on ppc & windows (so far):

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.
Resolution: FIXED → ---
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.
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))
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.
Or have an additional 4 byte padding member at the end and assert that size = sizeof int64 + sizeof type + sizeof padding.
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 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.)
Attachment #435041 - Flags: review?(roc)
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.