Closed
Bug 984125
Opened 10 years ago
Closed 10 years ago
Convert nsStringBundleService::mBundleMap to nsDataHashtable
Categories
(Core :: Internationalization, enhancement)
Core
Internationalization
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: mccr8, Assigned: anujagarwal464)
References
Details
Attachments
(1 file, 11 obsolete files)
6.00 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•10 years ago
|
Severity: normal → enhancement
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → anujagarwal464
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8400737 -
Flags: feedback?(continuation)
Reporter | ||
Comment 2•10 years ago
|
||
Comment on attachment 8400737 [details] [diff] [review] bug984125.diff Review of attachment 8400737 [details] [diff] [review]: ----------------------------------------------------------------- ::: intl/strres/src/nsStringBundle.cpp @@ -494,5 @@ > }; > > > nsStringBundleService::nsStringBundleService() : > - mBundleMap(MAX_CACHED_BUNDLES, true) One thing that concerns me here is that we're going from a threadsafe nsHashtable to a non-threadsafe one. I'm not sure if that's still needed. If it is, you may have to add a bunch of locks. I'll see what Ehsan thinks. @@ +656,5 @@ > // in the hashtable, so set up the cacheEntry > cacheEntry->mBundle = aBundle; > NS_ADDREF(cacheEntry->mBundle); > > + cacheEntry->mHashKey = (nsCString*)aHashKey; Does this make a copy of the string? Because that's what you want to do here, I think.
Attachment #8400737 -
Flags: feedback?(continuation)
Reporter | ||
Comment 3•10 years ago
|
||
Ehsan, do you know if nsStringBundleService still needs to be threadsafe? If it does, what should we do about this particular hashtable? I don't know how threadsafe it really is right now, as we're pulling random pointers out of the hashtable and using them without holding the lock.
Flags: needinfo?(ehsan)
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #2) > @@ +656,5 @@ > > // in the hashtable, so set up the cacheEntry > > cacheEntry->mBundle = aBundle; > > NS_ADDREF(cacheEntry->mBundle); > > > > + cacheEntry->mHashKey = (nsCString*)aHashKey; > > Does this make a copy of the string? Because that's what you want to do > here, I think. No this does not make a copy. How to get clone of nsCString?
Comment 5•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #3) > Ehsan, do you know if nsStringBundleService still needs to be threadsafe? > If it does, what should we do about this particular hashtable? I don't know > how threadsafe it really is right now, as we're pulling random pointers out > of the hashtable and using them without holding the lock. I don't know, maybe Benjamin knows?
Flags: needinfo?(ehsan) → needinfo?(benjamin)
Comment 6•10 years ago
|
||
The string bundle service is in fact not threadsafe. Please add threadsafety assertions and proceed as if it's mainthread-only.
Flags: needinfo?(benjamin)
Reporter | ||
Comment 7•10 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #6) > The string bundle service is in fact not threadsafe. Please add threadsafety > assertions and proceed as if it's mainthread-only. Thanks, Benjamin! Anuj, you could add this to your patch. I think you just need to change NS_DECL_THREADSAFE_ISUPPORTS to NS_DECL_ISUPPORTS in nsStringBundleService. > No this does not make a copy. How to get clone of nsCString? Looking at this some more, I think you can just get rid of the mHashKey field of bundleCacheEntry_t. I'm not entirely sure why the existing code needs this. Maybe it just does it to make it easier to delete the key once the entry is gone. Anyways, if you get rid of mHashKey, then I think the new hash table should deal with the string copying as needed.
Reporter | ||
Updated•10 years ago
|
Summary: Convert nsStringBundleService::mBundleMap to a modern hash table → Convert nsStringBundleService::mBundleMap to nsClassHashtable
Assignee | ||
Comment 8•10 years ago
|
||
http://mxr.mozilla.org/mozilla-central/source/intl/strres/src/nsStringBundle.cpp#642 If I get rid of mHashKey , then I won’t be able to check if mBundleMap contains cacheEntry or not because cacheEntry is received form mBundleCache.getLast(). mBundleMap.Contains(???)
Attachment #8400737 -
Attachment is obsolete: true
Attachment #8407407 -
Flags: feedback?(continuation)
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8407407 [details] [diff] [review] bug984125.diff Review of attachment 8407407 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, looks like I was wrong. This code is all really weird. It seems overly complicated. It is implementing a 16-entry cache with an LRU replacement policy using a linked list and a hashtable, and it is allocating those entries out of an arena allocator. But hey, maybe this code is very performance sensitive. Anyways, as you noted, the linked list contains the key, so that when we want to evict the oldest entry in the hashtable, it looks at the head of the list, then looks up the key to remove it. Other than the one mHashKey thing this looks good to me. ::: intl/strres/src/nsStringBundle.cpp @@ +656,5 @@ > // in the hashtable, so set up the cacheEntry > cacheEntry->mBundle = aBundle; > NS_ADDREF(cacheEntry->mBundle); > > + cacheEntry->mHashKey = (nsCString*)aHashKey; I don't think this is right. Maybe you can make the type of mHashKey to be nsCString. Then you should be able to just do |cacheEntry->mHashKey = aHashKey;| and I think it will just create the new string etc. in the right way? Let mw know how that goes. The string guide might be useful, though it has a lot of information so it can be hard to find what you are looking for: https://developer.mozilla.org/en-US/docs/Mozilla_internal_string_guide
Attachment #8407407 -
Flags: feedback?(continuation) → feedback-
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #9) > I don't think this is right. > > Maybe you can make the type of mHashKey to be nsCString. Then you should be > able to just do |cacheEntry->mHashKey = aHashKey;| and I think it will just > create the new string etc. in the right way? Let mw know how that goes. In last patch, type of mHashKey is nsCString. > struct bundleCacheEntry_t : public LinkedListElement<bundleCacheEntry_t> { > nsCString *mHashKey; > cacheEntry->mHashKey = (nsCString*)aHashKey; Type of aHashKey is char* therefore I need to typecast it before assigning it to cacheEntry->mHashKey. Could you tell me which method I should use to get a copy of aHashKey? https://developer.mozilla.org/en-US/docs/Mozilla_internal_string_guide#Memory_Allocation_-_how_to_avoid_it.2C_which_methods_to_use
Flags: needinfo?(continuation)
Reporter | ||
Comment 11•10 years ago
|
||
(In reply to Anuj Agarwal [:anujagarwal464] from comment #10) > In last patch, type of mHashKey is nsCString. Yeah, sorry, I guess I gave you bad advice about that. As you can tell, I'm not very familiar with strings. :) What changed my mind is that the string guide says "the overhead of the allocation outweighs the few bytes you'd save by keeping a pointer", so I think it makes sense to have the type be nsCString as you had before. I was worried about overhead by somehow keeping a copy around, but I think the nsCString will share the data appropriately, and just uses a pointer internally, or something like that, so I think it is fine. > Type of aHashKey is char* therefore I need to typecast it before assigning > it to cacheEntry->mHashKey. > Could you tell me which method I should use to get a copy of aHashKey? I think if mHashKey has type nsCString, then you can just assign it, and it will use the nsCString constructor that takes a char* as an argument: http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsStringAPI.h#891 Let me know if that doesn't work, though.
Flags: needinfo?(continuation)
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #11) > cacheEntry->mHashKey = aHashKey; Direct assignment will not work. Because types pointed to are unrelated. Conversion will requires reinterpret_cast, C-style cast or function-style cast. I compiled the patch with changes. Getting the error > cannot convert from 'const char *' to 'nsCString *'
Flags: needinfo?(continuation)
Reporter | ||
Comment 13•10 years ago
|
||
What if you change the type of mHashKey to nsCString?
Flags: needinfo?(continuation)
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #13) > What if you change the type of mHashKey to nsCString? Type of mHashKey is already nsCString.
Flags: needinfo?(continuation)
Reporter | ||
Comment 15•10 years ago
|
||
Can you upload the version of the patch that uses nsCString for mHashKey instead of nsCString*? I can see what it takes to get that one line working.
Flags: needinfo?(continuation)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8408423 -
Flags: feedback?
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8407407 -
Attachment is obsolete: true
Attachment #8408423 -
Attachment is obsolete: true
Attachment #8408423 -
Flags: feedback?
Attachment #8408776 -
Flags: review?(continuation)
Reporter | ||
Comment 18•10 years ago
|
||
Comment on attachment 8408776 [details] [diff] [review] bug984125.diff Review of attachment 8408776 [details] [diff] [review]: ----------------------------------------------------------------- It looks like you are almost there, but not quite yet. The comments below need to be addressed. Thanks for sticking with this, this code turned out to be trickier than I expected. ::: intl/strres/src/nsStringBundle.cpp @@ +584,5 @@ > nsresult > nsStringBundleService::getStringBundle(const char *aURLSpec, > nsIStringBundle **aResult) > { > + nsCString Key(aURLSpec); key instead of Key please. local variables should start with a lower case letter. I think this should also be nsAutoCString instead of nsCString. I think with nsAutoCString, the space will be allocated on the stack instead of the heap, which should be more efficient. That's okay here because this is just a local variable, not a field. @@ +646,2 @@ > #endif > + Please don't add trailing whitespace. ::: intl/strres/src/nsStringBundleService.h @@ +49,4 @@ > > static void recycleEntry(bundleCacheEntry_t*); > > + nsClassHashtable<nsCStringHashKey, bundleCacheEntry_t> mBundleMap; Did you try running the browser with this patch? When I tried running it with this, it was crashing. I think the type here needs to be nsDataHashtable<nsCStringHashKey, bundleCacheEntry_t*> instead. With an nsClassHashtable, any cache entry that gets removed will get a regular delete called on it, but the object was allocated out of mCacheEntryPool, so that's not okay. nsDataHashtable is okay because the code is already doing the work to manage the memory manually.
Attachment #8408776 -
Flags: review?(continuation) → review-
Assignee | ||
Comment 19•10 years ago
|
||
This patch is causing a crash.
Attachment #8408787 -
Flags: feedback?(continuation)
Assignee | ||
Comment 20•10 years ago
|
||
> aEntry->~bundleCacheEntry_t();
I think explicitly calling constructor in causing crash. Browser works fine after removing that line.
Flags: needinfo?(continuation)
Assignee | ||
Comment 21•10 years ago
|
||
Err, I meant destructor.
Reporter | ||
Comment 22•10 years ago
|
||
What do you do to make it crash? Just start up the browser? What is the stack like? Your patch seems to work for me. The problem is that without calling the destructor for the cache entry, we'll end up leaking any data owned by the string. You can see this because the "nsStringStats" that a debug build prints out when you exit will say you are leaking, or something like that.
Flags: needinfo?(continuation)
Reporter | ||
Updated•10 years ago
|
Attachment #8408787 -
Flags: feedback?(continuation)
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8408776 -
Attachment is obsolete: true
Attachment #8408787 -
Attachment is obsolete: true
Reporter | ||
Comment 24•10 years ago
|
||
Here's the conversion patch rebased over the patches in bug 997963. The main advantage is that it avoids the weirdness with explicitly calling a destructor in recycleEntry, because the patches in the other bug just eliminates recycleEntry altogether in favor more more standard stuff.
Reporter | ||
Comment 25•10 years ago
|
||
I think we can also just pass an nsCString to insertIntoCache, but I don't know how much of an improvement it is.
Reporter | ||
Comment 26•10 years ago
|
||
try run: https://tbpl.mozilla.org/?tree=Try&rev=5684ab4afbc3
Reporter | ||
Comment 27•10 years ago
|
||
Attachment #8409443 -
Attachment is obsolete: true
Attachment #8409444 -
Attachment is obsolete: true
Reporter | ||
Comment 28•10 years ago
|
||
I rebased these two patches again, now that bug 997963 has landed. Anuj, is the change in this patch okay with you?
Attachment #8410352 -
Flags: feedback?(anujagarwal464)
Assignee | ||
Comment 29•10 years ago
|
||
Comment on attachment 8410352 [details] [diff] [review] pass cstring into insertintocache Review of attachment 8410352 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. :) Instead of rebasing my patch you can delete it and submit a new patch.
Attachment #8410352 -
Flags: feedback?(anujagarwal464) → feedback+
Reporter | ||
Comment 30•10 years ago
|
||
This is Anuj's patch, I just did some rebasing here.
Attachment #8409426 -
Attachment is obsolete: true
Attachment #8410351 -
Attachment is obsolete: true
Attachment #8410352 -
Attachment is obsolete: true
Attachment #8410370 -
Flags: review?(ehsan)
Comment 31•10 years ago
|
||
Comment on attachment 8410370 [details] [diff] [review] Convert nsStringBundleService::mBundleMap to nsDataHashtable. Review of attachment 8410370 [details] [diff] [review]: ----------------------------------------------------------------- ::: intl/strres/src/nsStringBundle.cpp @@ +565,5 @@ > nsresult > nsStringBundleService::getStringBundle(const char *aURLSpec, > nsIStringBundle **aResult) > { > + nsAutoCString key(aURLSpec); Nit: please use nsDependentCString.
Attachment #8410370 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 32•10 years ago
|
||
Attachment #8410370 -
Attachment is obsolete: true
Attachment #8410462 -
Flags: review?(ehsan)
Reporter | ||
Comment 33•10 years ago
|
||
Comment on attachment 8410462 [details] [diff] [review] Convert nsStringBundleService::mBundleMap to nsDataHashtable. r=ehsan Review of attachment 8410462 [details] [diff] [review]: ----------------------------------------------------------------- All you did was fix the nit, so you don't need to request a re-review.
Attachment #8410462 -
Flags: review?(ehsan) → review+
Reporter | ||
Comment 34•10 years ago
|
||
Hooray! https://hg.mozilla.org/integration/mozilla-inbound/rev/069ed2015d14
Comment 35•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/069ed2015d14
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Reporter | ||
Comment 36•10 years ago
|
||
Thanks for sticking with this, Anuj! It was much trickier than I'd anticipated.
Assignee | ||
Updated•10 years ago
|
Summary: Convert nsStringBundleService::mBundleMap to nsClassHashtable → Convert nsStringBundleService::mBundleMap to nsDataHashtable
You need to log in
before you can comment on or make changes to this bug.
Description
•