Closed
Bug 1037264
Opened 10 years ago
Closed 10 years ago
Implement 8-bit scratch strings for XPConnect argument conversion
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(2 files, 3 obsolete files)
8.29 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
2.73 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
I've got a partial patch for this, which I'll finish up later tonight or tomorrow.
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8454785 -
Flags: review?(neil)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8454786 -
Flags: review?(neil)
Assignee | ||
Comment 4•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=8db71c6d828c
Comment 5•10 years ago
|
||
I waited for bug 1036777 to land on trunk and I still can't get the patches to apply...
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8455862 -
Flags: review?(neil)
Assignee | ||
Comment 8•10 years ago
|
||
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 9•10 years ago
|
||
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?
Assignee | ||
Comment 10•10 years ago
|
||
(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. :-(
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8455862 -
Attachment is obsolete: true
Attachment #8455862 -
Flags: review?(neil)
Attachment #8456458 -
Flags: review?(neil)
Comment 12•10 years ago
|
||
(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).
Comment 13•10 years ago
|
||
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 :-(
Comment 14•10 years ago
|
||
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 15•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8456458 -
Flags: review?(neil) → review+
Assignee | ||
Comment 16•10 years ago
|
||
(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).
Assignee | ||
Comment 17•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4bbe15384faa remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5a5d9f2a29ca
Comment 18•10 years ago
|
||
Sure, but the 2 is still a magic number.
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4bbe15384faa https://hg.mozilla.org/mozilla-central/rev/5a5d9f2a29ca
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•