Closed Bug 1037264 Opened 5 years ago Closed 5 years ago
Implement 8-bit scratch strings for XPConnect argument conversion
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.
I waited for bug 1036777 to land on trunk and I still can't get the patches to apply...
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 firstname.lastname@example.org 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. :-(
(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+
(In reply to email@example.com 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.
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.