Remove useAllocator nastiness in XPCConvert::JSData2Native

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: Ms2ger)

Tracking

unspecified
mozilla33
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

These branches look like this:

Right below here, we have the lines:

        if (useAllocator) {
            ws = nsXPConnect::GetRuntimeInstance()->NewShortLivedString();
            *((const nsString**)d) = ws;
        } else {
            ws = *((nsString**)d);
        }

This threw me for a loop when going over it, because that cast looks really unsafe. It turns out it is, and the issue is just that we never call this with useAllocator=false for any dipper parameters (which include DOMStrings). We should remove these conditionals, and replace them with MOZ_ASSERTs.

I'll mark this as dependent on bug 1034627 to make jandem's life easier.
(In reply to Bobby Holley from comment #0)
>         if (useAllocator) {
>             ws = nsXPConnect::GetRuntimeInstance()->NewShortLivedString();
>             *((const nsString**)d) = ws;
>         } else {
>             ws = *((nsString**)d);
>         }
> 
> This threw me for a loop when going over it, because that cast looks really
> unsafe. It turns out it is, and the issue is just that we never call this
> with useAllocator=false for any dipper parameters (which include
> DOMStrings). We should remove these conditionals, and replace them with
> MOZ_ASSERTs.

OK, I cheated. Originally there were two code paths (with slightly different code, which was one of the issues I was trying to fix) one saying nsString *ws = nsXPConnect::GetRuntimeInstance()->NewShortLivedString(); *((const nsString**)d) = ws; and the other saying nsAString *ws = *((nsAString**)d); when in fact what I should have written was:
nsAString *ws;
if (useAllocator) {
  nsString *w = nsXPConnect::GetRuntimeInstance()->NewShortLivedString();
  *((const nsString**)d) = ws;
  ws = w;
} else {
  ws = *((nsAString**)d);
}
(Note that this compiles because I only ever call nsAString methods on *ws). But I'm sure that both code paths are used; IIRC one is used for calling C++ from JS and the other is for returning strings from C++ to JS.
Assignee: nobody → Ms2ger
OS: Mac OS X → All
Hardware: x86 → All
It turns out the useAllocator argument is only used for the dipper types
(nsXPTType::{T_ASTRING, T_DOMSTRING, T_UTF8STRING, T_CSTRING}), while we
only pass true in cases where we don't have a dipper type:

* XPCConvert::JSArray2Native errors on those types;
* GetNamedPropertyAsVariantRaw() passes an interface type;
* nsXPCWrappedJSClass::CallMethod passes !param.IsDipper() for the first
  calls and only reaches the last call for dependent types, which do not
  include any of the dipper types;
* CallMethodHelper::ConvertIndependentParam handles dipper types earlier
* and CallMethodHelper::ConvertDependentParam handles dependent types.
Attachment #8453720 - Flags: review?(bobbyholley)
(In reply to comment #3)
> useAllocator = true

This happened to be a T_UTF8STRING in this case.

(In reply to comment #4)
> useAllocator = false

This happened to be a T_CSTRING in this case.
Comment on attachment 8453720 [details] [diff] [review]
Part 2 - Remove the useAllocator argument to XPCConvert::JSData2Native

Review of attachment 8453720 [details] [diff] [review]:
-----------------------------------------------------------------

Ahah! Thanks for sorting this out Ms2ger.

The useAllocator stuff must have become dead when I factored out JS->C++ call dipper conversion into CallMethodHelper::HandleDipperParam in bug 683802. Happy day!
Attachment #8453720 - Flags: review?(bobbyholley) → review+
Summary: Remove bogus useAllocator branches for DOMString, UTF8String, and CString in XPCConvert::JSData2Native → Remove useAllocator nastiness in XPCConvert::JSData2Native
Attachment #8453720 - Attachment description: Remove the useAllocator argument to XPCConvert::JSData2Native → Part 2 - Remove the useAllocator argument to XPCConvert::JSData2Native
Comment on attachment 8453984 [details] [diff] [review]
Part 1 - Stop relying on useAllocator for 'in' string classes in ConvertIndependentParam. v1

>+    if (paramInfo.IsStringClass() && !AllocateStringClass(dp, paramInfo))
This undoes bug 966911 part 1 :-(
(What's so nasty about useAllocator anyway?)
Comment on attachment 8453984 [details] [diff] [review]
Part 1 - Stop relying on useAllocator for 'in' string classes in ConvertIndependentParam. v1

Review of attachment 8453984 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I don't think I feel confident in stamping this.
Attachment #8453984 - Flags: review?(Ms2ger)
(In reply to :Ms2ger from comment #7)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/69c0707a144a

For the record: backed out in

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6553613d42d

because IsDipper() is false for in parameters.
(In reply to neil@parkwaycc.co.uk from comment #9)
> Comment on attachment 8453984 [details] [diff] [review]
> Part 1 - Stop relying on useAllocator for 'in' string classes in
> ConvertIndependentParam. v1
> 
> >+    if (paramInfo.IsStringClass() && !AllocateStringClass(dp, paramInfo))
> This undoes bug 966911 part 1 :-(

Oh, true. But only a small part of it. The NewShortLivedString stuff means that we still don't allocate for the 16-bit cases, so we're only allocating for the 8-bit cases. And there were various other improvements in that patch too.

As I understand it, the optimization loss of Part 1 here is that we'll have an extra allocation for null/void/empty cstring and utf8string parameters. Is there anything else?

It seems like if we care about this case, we should just add an analogous NewShortLivedCString. But I suspect that we don't.

(In reply to neil@parkwaycc.co.uk from comment #10)
> (What's so nasty about useAllocator anyway?)

It makes this function absurdly complex. Each time I've had to review a patch to it (twice this year), it's taken me upwards of an hour to page everything in.

With the WebIDL Codegen, this stuff isn't hot in the same way that it used to be. At this point, simplicity and hackability is much more of a priority. Unless we've measured an impact from these optimizations (which bug 966911 comment 1 suggests we haven't), we should steer towards making this code comprehensible, which these two patches definitely do.
Attachment #8453984 - Flags: review?(neil)
Comment on attachment 8453984 [details] [diff] [review]
Part 1 - Stop relying on useAllocator for 'in' string classes in ConvertIndependentParam. v1

So you're going to clean up all the useAllocator = true cases too? (And by that I don't mean Ms2ger's patch, as it doesn't clean up all the cases.)

>-    if (!XPCConvert::JSData2Native(&dp->val, src, type, true, &param_iid, &err)) {
>+    if (!XPCConvert::JSData2Native(&dp->val, src, type, !paramInfo.IsStringClass(), &param_iid, &err)) {
Why this change?
(In reply to neil@parkwaycc.co.uk from comment #15)
> Comment on attachment 8453984 [details] [diff] [review]
> Part 1 - Stop relying on useAllocator for 'in' string classes in
> ConvertIndependentParam. v1
> 
> So you're going to clean up all the useAllocator = true cases too? (And by
> that I don't mean Ms2ger's patch, as it doesn't clean up all the cases.)

Which cases doesn't it clean up? The whole param is gone.

> >-    if (!XPCConvert::JSData2Native(&dp->val, src, type, true, &param_iid, &err)) {
> >+    if (!XPCConvert::JSData2Native(&dp->val, src, type, !paramInfo.IsStringClass(), &param_iid, &err)) {
> Why this change?

So that Ms2ger's patch (as part 2) is pure dead-code removal.
(In reply to Bobby Holley from comment #16)
> (In reply to comment #15)
> > So you're going to clean up all the useAllocator = true cases too? (And by
> > that I don't mean Ms2ger's patch, as it doesn't clean up all the cases.)
> 
> Which cases doesn't it clean up? The whole param is gone.

Sure, but there's a bunch of code that's called by those cases.

> > >-    if (!XPCConvert::JSData2Native(&dp->val, src, type, true, &param_iid, &err)) {
> > >+    if (!XPCConvert::JSData2Native(&dp->val, src, type, !paramInfo.IsStringClass(), &param_iid, &err)) {
> > Why this change?
> 
> So that Ms2ger's patch (as part 2) is pure dead-code removal.

So why didn't you just change it to pass false like the other callers?
Comment on attachment 8453720 [details] [diff] [review]
Part 2 - Remove the useAllocator argument to XPCConvert::JSData2Native

>+        nsString* ws = *((nsString**)d);
By the way, as per comment #1, this is only really guaranteed to be an nsAString*.
I tried to gather some quick-and-dirty statistics, and they seem to agree with you.

244 null strings returned from JS to C++.
13 null strings passed from JS to C++.
68 empty strings returned from JS to C++.
7 null strings passed from JS to C++.
117 other strings returned from JS to C++.
1271 other strings passed from JS to C++.

So just over 1% of allocations were saved.

The most popular string type was UTF8String with 860 strings (50%!); AString had 376, DOMString had 357, CString had 127. Maybe we do need an 8-bit short lived string.
(In reply to neil@parkwaycc.co.uk from comment #17)
> (In reply to Bobby Holley from comment #16)
> > (In reply to comment #15)
> > > So you're going to clean up all the useAllocator = true cases too? (And by
> > > that I don't mean Ms2ger's patch, as it doesn't clean up all the cases.)
> > 
> > Which cases doesn't it clean up? The whole param is gone.
> 
> Sure, but there's a bunch of code that's called by those cases.

I still don't understand. My plan is to land my patch with Ms2ger's patch on top of it (see the try push in comment 13). Which cases code remain with those two patches applied?

> 
> > > >-    if (!XPCConvert::JSData2Native(&dp->val, src, type, true, &param_iid, &err)) {
> > > >+    if (!XPCConvert::JSData2Native(&dp->val, src, type, !paramInfo.IsStringClass(), &param_iid, &err)) {
> > > Why this change?
> > 
> > So that Ms2ger's patch (as part 2) is pure dead-code removal.
> 
> So why didn't you just change it to pass false like the other callers?

Shrug - just so that the patch is more verifiably correct by inspection. They're equivalent IIUC, and it's a moot point with both patches applied.
(In reply to neil@parkwaycc.co.uk from comment #18)
> Comment on attachment 8453720 [details] [diff] [review]
> Part 2 - Remove the useAllocator argument to XPCConvert::JSData2Native
> 
> >+        nsString* ws = *((nsString**)d);
> By the way, as per comment #1, this is only really guaranteed to be an
> nsAString*.

Good point. I'll attach an additional patch.

(In reply to neil@parkwaycc.co.uk from comment #19)
> The most popular string type was UTF8String with 860 strings (50%!); AString
> had 376, DOMString had 357, CString had 127. Maybe we do need an 8-bit short
> lived string.

Good find! This probably changed a lot because most of the APIs with DOMStrings are now on WebIDL. I'll file a followup bug.
Blocks: 1037264
These types map to nsAString and nsACString respectively.
Attachment #8454183 - Flags: review?(neil)
(In reply to Bobby Holley from comment #20)
> Which cases code remain with those two patches applied?

Actually there was less code than I thought, but both in CleanupParam and DeleteShortLivedString there's code to handle the cases you're removing.
Attachment #8454183 - Flags: review?(neil) → review+
Now with more code removal.
Attachment #8453984 - Attachment is obsolete: true
Attachment #8453984 - Flags: review?(neil)
Attachment #8454783 - Flags: review?(neil)
Comment on attachment 8454783 [details] [diff] [review]
Part 1 - Stop relying on useAllocator for 'in' string classes in ConvertIndependentParam. v2

>+    // String classes are always "in" - those that are marked "out" are converted
>+    // by the XPIDL compiler to "in+dipper".
>+    if (paramInfo.IsStringClass() && !AllocateStringClass(dp, paramInfo))
>+        return false;
>     if (paramInfo.IsDipper())
>-        return HandleDipperParam(dp, paramInfo);
>+        return true; // See the note above IsDipper in xptinfo.h.
(Is it worth nesting the AllocateStringClass and IsDipper checks inside an IsStringClass block?)
Attachment #8454783 - Flags: review?(neil) → review+
Comment on attachment 8454783 [details] [diff] [review]
Part 1 - Stop relying on useAllocator for 'in' string classes in ConvertIndependentParam. v2

>+
>     // Whether this parameter is passed indirectly on the stack. This mainly
>     // applies to out/inout params, but we use it unconditionally for certain
>     // types.
>     bool IsIndirect() const {return IsOut() ||
>                                GetType().TagPart() == nsXPTType::T_JSVAL;}
> 
>     // NOTE: other activities on types are done via methods on nsIInterfaceInfo
(This patch only has 7 lines of context at the bottom when 8 are expected; the patch tool on one of my build environments is unhappy with this.)
Comment on attachment 8453720 [details] [diff] [review]
Part 2 - Remove the useAllocator argument to XPCConvert::JSData2Native

(Has this bitrotted? I seem to need it for bug 1037264 but I can't get it to apply.)
(In reply to neil@parkwaycc.co.uk from comment #27)
> Comment on attachment 8453720 [details] [diff] [review]
> Part 2 - Remove the useAllocator argument to XPCConvert::JSData2Native
> 
> (Has this bitrotted? I seem to need it for bug 1037264 but I can't get it to
> apply.)

Yes, it was bitrotted by jandem's patch. I unbitrotted it here:

https://tbpl.mozilla.org/?tree=Try&rev=8db71c6d828c
You need to log in before you can comment on or make changes to this bug.