1187517 - Factor out whether an entry needs rekeying into yet another template parameter for TraceableHashMap

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Factor out whether an entry needs rekeying into yet another template parameter for TraceableHashMap

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4f8507bee27

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 8638818 [details] [diff] [review]
Factor out whether an entry needs rekeying into yet another template parameter for TraceableHashMap

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

Please add a test to js/src/jsapi-tests/testGCExactRooting.cpp next to the other TraceableHashMap tests.

::: js/public/HashTable.h
@@ +651,5 @@
>  
>      template <class, class, class> friend class detail::HashTable;
>      template <class> friend class detail::HashTableEntry;
>      template <class, class, class, class> friend class HashMap;
> +    template <class, class, class, class, class, class, class> friend class TraceableHashMap;

This is quite ugly, but I guess we can't get around it.

::: js/src/jsgc.cpp
@@ +7479,5 @@
> +      default:
> +        MOZ_CRASH("Unexpected JS::TraceKind in DefaultNeedsRekey<JS::GCCellPtr>");
> +        return false;
> +    }
> +}

This should be a one liner. The following works for me:

struct IsForwardedFunctor {
    template <typename T> bool operator()(T* t) { return IsForwarded(t); }
};

/* static */ bool
DefaultNeedsRekey<JS::GCCellPtr>::needsRekey(JS::GCCellPtr& p)
{
    return CallTyped(IsForwardedFunctor(), p.asCell(), p.kind());
}

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Factor out whether an entry needs rekeying into yet another template parameter for TraceableHashMap

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f1b0db97dc4

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Factor out whether an entry needs rekeying into yet another template parameter for TraceableHashMap

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Sorry for the churn. This should fix "error: specialization of 'template<class> struct js::DefaultHasher' in different namespace [-fpermissive]" in the jsapi tests.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a1092d07352

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8639421 [details] [diff] [review]
Factor out whether an entry needs rekeying into yet another template parameter for TraceableHashMap

Need to figure out the SM(r) failures.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Factor out whether an entry needs rekeying into yet another template parameter for TraceableHashMap

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Seems to be working after a rebase and some tweaks...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12b9409cfd5b

Comment 11

[Terrence Cole [:terrence]]

Comment on attachment 8645886 [details] [diff] [review]
Factor out whether an entry needs rekeying into yet another template parameter for TraceableHashMap

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

I'm still sad we need this infrastructure, but we don't have much choice until we can fix the object keying issue.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Gah that SM(r) failure is still there, I was just using the wrong zeal settings :(

Going to put this on the backburner for a while and move forward on bug 1187062 without this.

Comment 13

[Terrence Cole [:terrence]]

Great! The uniqueID system is on tip now, so in theory we should not need this anymore.