Create method nsSMILValue::Swap()

RESOLVED FIXED in mozilla1.9.3a4

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla1.9.3a4
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

9 years ago
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
(Assignee)

Comment 1

9 years ago
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()?)
(Assignee)

Comment 2

9 years ago
Created attachment 434633 [details] [diff] [review]
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.)
Attachment #434633 - Flags: review?(roc)
(Assignee)

Updated

9 years ago
Summary: Create method nsISMILType::TransferValue() (or Swap()?) → Create method nsSMILValue::Swap()
(Assignee)

Comment 3

9 years ago
Created attachment 434639 [details] [diff] [review]
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.
(Assignee)

Comment 4

9 years ago
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
(Assignee)

Comment 5

9 years ago
Created attachment 434641 [details] [diff] [review]
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)
(Assignee)

Updated

9 years ago
Attachment #434641 - Flags: review?(roc)
In part 1, why not just implement it directly with four assignments instead of going through TransferValue?
(Assignee)

Comment 7

9 years ago
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?
(Assignee)

Comment 8

9 years ago
Created attachment 434664 [details] [diff] [review]
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.
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.
(Assignee)

Comment 10

9 years ago
I think the existing PR_STATIC_ASSERT already takes care of that, by checking sizeof(PRUint64) + sizeof(mType pointer) = sizeof(nsSMILValue)
Right, sorry.
(Assignee)

Comment 12

9 years ago
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=a4178a154d16
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=1bcf725bb32f
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
(Assignee)

Comment 13

9 years ago
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.
Status: RESOLVED → REOPENED
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.
(Assignee)

Comment 15

9 years ago
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))
(Assignee)

Comment 16

9 years ago
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.
(Assignee)

Comment 18

9 years ago
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.
(Assignee)

Comment 19

9 years ago
Created attachment 435041 [details] [diff] [review]
patch 1 v3: Add nsSMILValue::Swap
Attachment #434664 - Attachment is obsolete: true
(Assignee)

Comment 20

9 years ago
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)
(Assignee)

Comment 21

9 years ago
Re-landed:
 http://hg.mozilla.org/mozilla-central/rev/62ea9c99b289
 http://hg.mozilla.org/mozilla-central/rev/6eaaeb9c35e2
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.