772292 - Convert JSObject2WrappedJSMap to a new-style HashTable

Status: RESOLVED FIXED

Description

[Terrence Cole [:terrence]]

Created attachment 640441 [details] [diff] [review]
v0

Try run up at:
https://tbpl.mozilla.org/?tree=Try&rev=ba57767df1f9

Comment 1

[:Ms2ger (⌚ UTC+1/+2)]

Comment on attachment 640441 [details] [diff] [review]
v0

Review of attachment 640441 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCMaps.cpp
@@ +93,5 @@
> +                if (JS_IsAboutToBeFinalized(wrapper->GetJSObjectPreserveColor()))
> +                    dying->AppendElement(wrapper);
> +            }
> +        }
> +        wrapper = wrapper->GetNextWrapper();

This line should be inside the while loop.

::: js/xpconnect/src/XPCMaps.h
@@ +604,5 @@
>      typedef js::HashMap<JSObject *, JSObject *, js::PointerHasher<JSObject *, 3>,
>                          js::SystemAllocPolicy> Map;
>  
>  public:
> +    static JSObject2JSObjectMap* newMap(int size) {

Unrelated?

@@ +664,5 @@
>      }
>  
>  private:
>      JSObject2JSObjectMap() MOZ_DELETE;
> +    JSObject2JSObjectMap(int size) {}

Should remove this constructor, then.

Comment 2

[Terrence Cole [:terrence]]

Created attachment 640653 [details] [diff] [review]
v1: With fixes.

(In reply to :Ms2ger from comment #1)
> ::: js/xpconnect/src/XPCMaps.cpp
> @@ +93,5 @@
> > +                if (JS_IsAboutToBeFinalized(wrapper->GetJSObjectPreserveColor()))
> > +                    dying->AppendElement(wrapper);
> > +            }
> > +        }
> > +        wrapper = wrapper->GetNextWrapper();
> 
> This line should be inside the while loop.

Good catch.  I made the mistake of trying to understand what this loop was doing instead of just copying it.
 
> ::: js/xpconnect/src/XPCMaps.h
> @@ +604,5 @@
> >      typedef js::HashMap<JSObject *, JSObject *, js::PointerHasher<JSObject *, 3>,
> >                          js::SystemAllocPolicy> Map;
> >  
> >  public:
> > +    static JSObject2JSObjectMap* newMap(int size) {
> 
> Unrelated?

A cleanup of the previous jsdhash I squashed.  I figured while I was here I would clean this up a bit more.

> @@ +664,5 @@
> >      }
> >  
> >  private:
> >      JSObject2JSObjectMap() MOZ_DELETE;
> > +    JSObject2JSObjectMap(int size) {}
> 
> Should remove this constructor, then.

Was the whole point of the above change.  Did it for the JSObject2WrappedJS but forgot it here for some reason.

Comment 3

[Terrence Cole [:terrence]]

Previous try run appears to have happened in the middle of complete infrastructure havoc:
https://tbpl.mozilla.org/?tree=Try&rev=08c73baedbee

Comment 4

[Blake Kaplan (:mrbkap)]

Comment on attachment 640653 [details] [diff] [review]
v1: With fixes.

Review of attachment 640653 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCMaps.h
@@ -74,5 @@
>  
> -    ~JSObject2WrappedJSMap();
> -private:
> -    JSObject2WrappedJSMap();    // no implementation
> -    JSObject2WrappedJSMap(int size);

Should the constructor stay private here to make sure consumers use newMap?

@@ -668,5 @@
>          }
>      }
>  
>  private:
> -    JSObject2JSObjectMap() MOZ_DELETE;

Ditto here.

Comment 5

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff15e729ec86

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ff15e729ec86