Closed Bug 984125 Opened 6 years ago Closed 6 years ago

Convert nsStringBundleService::mBundleMap to nsDataHashtable

Categories

(Core :: Internationalization, enhancement)

enhancement
Not set

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.
Severity: normal → enhancement
Assignee: nobody → anujagarwal464
Attached patch bug984125.diff (obsolete) — Splinter Review
Attachment #8400737 - Flags: feedback?(continuation)
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)
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)
(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?
(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)
The string bundle service is in fact not threadsafe. Please add threadsafety assertions and proceed as if it's mainthread-only.
Flags: needinfo?(benjamin)
(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.
Summary: Convert nsStringBundleService::mBundleMap to a modern hash table → Convert nsStringBundleService::mBundleMap to nsClassHashtable
Attached patch bug984125.diff (obsolete) — Splinter Review
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)
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-
(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)
(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)
(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)
What if you change the type of mHashKey to nsCString?
Flags: needinfo?(continuation)
(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)
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)
Attached patch bug984125.diff - not working (obsolete) — Splinter Review
Attachment #8408423 - Flags: feedback?
Attached patch bug984125.diff (obsolete) — Splinter Review
Attachment #8407407 - Attachment is obsolete: true
Attachment #8408423 - Attachment is obsolete: true
Attachment #8408423 - Flags: feedback?
Attachment #8408776 - Flags: review?(continuation)
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-
Attached patch bug984125.diff (obsolete) — Splinter Review
This patch is causing a crash.
Attachment #8408787 - Flags: feedback?(continuation)
> aEntry->~bundleCacheEntry_t();

I think explicitly calling constructor in causing crash. Browser works fine after removing that line.
Flags: needinfo?(continuation)
Err, I meant destructor.
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)
Attachment #8408787 - Flags: feedback?(continuation)
Attached patch bug984125.diff (obsolete) — Splinter Review
Attachment #8408776 - Attachment is obsolete: true
Attachment #8408787 - Attachment is obsolete: true
Depends on: 997963
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.
I think we can also just pass an nsCString to insertIntoCache, but I don't know how much of an improvement it is.
Attachment #8409443 - Attachment is obsolete: true
Attachment #8409444 - Attachment is obsolete: true
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)
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+
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 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+
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+
https://hg.mozilla.org/mozilla-central/rev/069ed2015d14
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Depends on: 1000190
Thanks for sticking with this, Anuj!  It was much trickier than I'd anticipated.
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.