Closed Bug 1037264 Opened 5 years ago Closed 5 years ago

Implement 8-bit scratch strings for XPConnect argument conversion

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(2 files, 3 obsolete files)

See bug 1036777 comment 19. We have mScratchStrings for nsAString, but nothing for nsACString. This should be straightforward to add, and will save us a lot of allocations.
I've got a partial patch for this, which I'll finish up later tonight or tomorrow.
Attachment #8454786 - Flags: review?(neil)
I waited for bug 1036777 to land on trunk and I still can't get the patches to apply...
Attachment #8454785 - Attachment is obsolete: true
Attachment #8454786 - Attachment is obsolete: true
Attachment #8454785 - Flags: review?(neil)
Attachment #8454786 - Flags: review?(neil)
Attachment #8455861 - Flags: review?(neil)
Attachment #8455862 - Flags: review?(neil)
I just rebased the patches, not sure if it'll help with the apply issues. Here are the patches pushed to github too, if that helps: https://github.com/bholley/gecko-dev/tree/killuseallocator
Comment on attachment 8455862 [details] [diff] [review]
Part 2 - Add an 8-bit short-lived string cache. v1

>+    ShortLivedStringBuffer<nsString> mScratchCStrings;
Did you mean nsCString?
(In reply to neil@parkwaycc.co.uk from comment #9)
> Comment on attachment 8455862 [details] [diff] [review]
> Part 2 - Add an 8-bit short-lived string cache. v1
> 
> >+    ShortLivedStringBuffer<nsString> mScratchCStrings;
> Did you mean nsCString?

So I did! I'm rather appalled that this worked on try. :-(
Attachment #8455862 - Attachment is obsolete: true
Attachment #8455862 - Flags: review?(neil)
Attachment #8456458 - Flags: review?(neil)
(In reply to Bobby Holley from comment #10)
> I'm rather appalled that this worked on try. :-(

AllocateStringClass doesn't care because it's writing into a void pointer.

CleanupParam doesn't care because it's got the wrong cast.

And JSData2Native doesn't care because the internal layout is almost identical (I can't remember whether EmptyCString().get() == (char *)EmptyString().get() in which case the internal layout is identical).
In my simple test I'm seeing 1 in 24 nsCString cache misses as opposed to 1 in 75 nsString cache misses. (And there were more nsCStrings than nsStrings anyway). I tried templating the cache on the number of short lived strings but I completely broke the build somehow :-(
Fixed my code and made some better measurements.
Number of scratch strings: 1    2    3    4
   nsCString cache misses: 7.3% 4.2% 1.8% 1.3%
    nsString cache misses: 2.7% 1.3% 1.0% 0.8%
(10112 total strings created in that test run)
Comment on attachment 8455861 [details] [diff] [review]
Part 1 - Factor out short-lived string buffer into a templated helper class. v1

>-#define XPCCCX_STRING_CACHE_SIZE 2
[I rather like the idea of having a named constant for this.]
Attachment #8455861 - Flags: review?(neil) → review+
Attachment #8456458 - Flags: review?(neil) → review+
(In reply to neil@parkwaycc.co.uk from comment #15)
> Comment on attachment 8455861 [details] [diff] [review]
> Part 1 - Factor out short-lived string buffer into a templated helper class.
> v1
> 
> >-#define XPCCCX_STRING_CACHE_SIZE 2
> [I rather like the idea of having a named constant for this.]

The current name is bad because it implies something about XPCCallContext (where this cache used to live, but doesn't anymore). And since this all lives in a header now, I decided to avoid polluting the namespace given that we only need to use the constant once (thanks to ArrayLength).
Sure, but the 2 is still a magic number.
https://hg.mozilla.org/mozilla-central/rev/4bbe15384faa
https://hg.mozilla.org/mozilla-central/rev/5a5d9f2a29ca
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.