815931 - Split out parts of jsgc.cpp into separate files

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: move root marking

This first patch moves some of the root marking code into a separate file.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: move some Auto classes to header

This patch moves some of the auto classes used for tracing into headers. It also calls RecordNativeStackTop automatically from one of the auto classes. This isn't needed by all the tracers, but it's not very expensive and it simplifies things.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: move iteration code

This patch moves a bunch of heap iteration code into its own file.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: use AutoPrepareForTracing in verifier routines

This just reuses AutoPrepareForTracing for the verifier.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: move verifier

This moves the verifier to its own file.

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 685923 [details] [diff] [review]
move some Auto classes to header

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

Nice, looks good.

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 685924 [details] [diff] [review]
move iteration code

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

Ditto.

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 685922 [details] [diff] [review]
move root marking

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

Nice!

::: js/src/gc/RootMarking.cpp
@@ +36,5 @@
> +typedef RootedValueMap::Entry RootEntry;
> +typedef RootedValueMap::Enum RootEnum;
> +
> +static inline bool
> +InFreeList(ArenaHeader *aheader, uintptr_t addr)

Whoops!  I should have added the new exact marking code /above/ this function.  Could you move InFreeList down so that it is next to the conservative marking instead of the exact marking?

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 685925 [details] [diff] [review]
use AutoPrepareForTracing in verifier routines

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

::: js/src/gc/Iteration.cpp
@@ -23,5 @@
>  {
>      JS_ASSERT(!IS_GC_MARKING_TRACER(trc));
>  
>      AutoPrepareForTracing prep(trc->runtime);
> -    RecordNativeStackTopForGC(trc->runtime);

I assume this has moved somewhere else since I last poked around here?

::: js/src/jsgc.cpp
@@ +4685,4 @@
>          return;
> +    }
> +
> +    AutoPrepareForTracing prep(rt);

Ooooh, shiny!  I really should have read your cleanup patch when it landed.

Comment 9

[Terrence Cole [:terrence]]

Comment on attachment 685926 [details] [diff] [review]
move verifier

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

I don't see Verifier.cpp in this patch.  Could you hg add it and re-upload?

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: move verifier, v2

Sorry, here it is with Verifier.cpp.

Comment 11

[Terrence Cole [:terrence]]

Comment on attachment 686266 [details] [diff] [review]
move verifier, v2

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

Unsurprisingly, there were no surprises :-).  Probably should have just r+'ed it sight-unseen.

Comment 12

[Terrence Cole [:terrence]]

Comment on attachment 686266 [details] [diff] [review]
move verifier, v2

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

Unsurprisingly, there were no surprises :-).  Probably should have just r+'ed it sight-unseen.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe5182d6786
https://hg.mozilla.org/integration/mozilla-inbound/rev/e94baab35c63
https://hg.mozilla.org/integration/mozilla-inbound/rev/15115c5169da
https://hg.mozilla.org/integration/mozilla-inbound/rev/1edf3b8879cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/07ce116d5ee8

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7fe5182d6786
https://hg.mozilla.org/mozilla-central/rev/e94baab35c63
https://hg.mozilla.org/mozilla-central/rev/15115c5169da
https://hg.mozilla.org/mozilla-central/rev/1edf3b8879cf
https://hg.mozilla.org/mozilla-central/rev/07ce116d5ee8

Comment 15

[Julian Seward [:jseward]]

I think this might have broken the --enable-valgrind stack-scan 
annotations.  I now get a whole bunch of error reports when GC runs,
which go away when I add the patch in the next comment.

Comment 16

[Julian Seward [:jseward]]

Attachment: Fix up valgrind stack-scan annotations

Comment 17

[Terrence Cole [:terrence]]

Julian, thanks for the patch!  I noticed those too, but didn't have time to investigate.

Comment 18

[Terrence Cole [:terrence]]

Comment on attachment 689194 [details] [diff] [review]
Fix up valgrind stack-scan annotations

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

I talked this over with Bill just now and we are agreed that we want to just depend on MOZ_VALGRIND and remove JS_VALGRIND, since we're just aliasing them anyway. This should be as simple as changing the #ifdef JS_VALGRIND in jsgc.cpp, gc/Verifier.cpp, and gc/RootMarking.cpp to MOZ_VALGRIND and simplifying the #include protection accordingly.  R+ with the above trivial changes and the nit below.

::: js/src/gc/RootMarking.cpp
@@ +30,5 @@
> +#ifdef MOZ_VALGRIND
> +# define JS_VALGRIND
> +#endif
> +#ifdef JS_VALGRIND
> +# include <valgrind/memcheck.h>

It looks like we also should have this include in gc/Verifier.cpp.

Comment 19

[Ralph Giles (:rillian)]

Attachment: Fix up valgrind stack-scan annotations

I wanted to test this so I went ahead an updated the nits. I'm not jseward, so please verify. :)

BTW, I don't see any valgrind markup in jsgc.cpp. Does it still need the include?

Comment 20

[Terrence Cole [:terrence]]

Comment on attachment 689371 [details] [diff] [review]
Fix up valgrind stack-scan annotations

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

Awesome! \o/

Comment 21

[Terrence Cole [:terrence]]

(In reply to Ralph Giles (:rillian) from comment #19)
> BTW, I don't see any valgrind markup in jsgc.cpp. Does it still need the
> include?

Good catch: probably not.  You can go ahead and remove it.

Comment 22

[Ralph Giles (:rillian)]

Attachment: Fix up valgrind stack-scan annotations

Done, thanks. Carrying forward previous r+.

Can someone please push the valgrind fix to incoming? I don't have level 2.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90d318029451

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/90d318029451