1293419 - Make the trace logger use js::Thread instead of PRThread

Status: RESOLVED FIXED

Description

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

This also introduces a hasher for js::Thread::Id.

Comment 1

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

Created attachment 8779066 [details] [diff] [review]
Make the trace logger use js::Thread instead of PRThread

This also introduces a hasher for js::Thread::Id which could certainly use a
good double checking. I wasn't totally sure which parts of the Id are actually
unique and/or belong in the hash.

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8779066 [details] [diff] [review]
Make the trace logger use js::Thread instead of PRThread

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

::: js/src/threading/Thread.h
@@ +83,5 @@
> +    }
> +
> +    static void rekey(Id& key, const Id& newKey) {
> +      key = newKey;
> +    }

Please remove |rekey|. It should not be instantiated unless you try to actually use rekeying and we *really* want the compile to fail if you think you need to change a Thread's id.

::: js/src/threading/posix/Thread.cpp
@@ +39,5 @@
>  
> +/* static */ js::HashNumber
> +js::Thread::Hasher::hash(const Lookup& l)
> +{
> +  return mozilla::HashBytes(l.platformData()->ptThread, sizeof(pthread_t));

|ptThread| is a pthread_t, so you'll need an & in front of it to call HashBytes.

Comment 3

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

Created attachment 8779512 [details] [diff] [review]
Make the trace logger use js::Thread instead of PRThread

Applied review feedback.

Comment 4

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

Try push for this and others: https://treeherder.mozilla.org/#/jobs?repo=try&revision=97ba2ddf4876

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bf671f59015
Make the trace logger use js::Thread instead of PRThread; r=terrence

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5bf671f59015