Prefer use of 'trace' to 'mark'

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(6 attachments, 1 obsolete attachment)

Assignee

Description

3 years ago
At the moment a bunch of stuff that does tracing is still called 'mark'.  We should use the name 'trace' for everything that takes a JSTracer.
Assignee

Comment 1

3 years ago
The rule will be:
 - a method that traces all contained GC things will be called 'trace' and take a JSTracer*
 - a method that's specific to GC marking will have the word 'mark' in the name and take a GCMarker*
Assignee

Comment 2

3 years ago
Rename methods in builtin directory.
Assignee: nobody → jcoppeard
Attachment #8813119 - Flags: review?(jdemooij)
Assignee

Comment 3

3 years ago
Rename methods in the parser directory.
Attachment #8813121 - Flags: review?(jdemooij)
Assignee

Comment 4

3 years ago
Posted patch trace-3-jitSplinter Review
Rename methods in jit directory.

'markUnconditionally' methods got renamed to just 'trace' losing the 'unconditionally' because trace methods usually trace everything unconditionally anyway.
Attachment #8813132 - Flags: review?(hv1989)
Assignee

Comment 5

3 years ago
Posted patch trace-5-src (obsolete) — Splinter Review
Rename methods in the vm directory.
Attachment #8813133 - Flags: review?(jwalden+bmo)
Assignee

Comment 6

3 years ago
Posted patch trace-4-vmSplinter Review
Rename methods in the vm directory (with the right patch this time).
Attachment #8813133 - Attachment is obsolete: true
Attachment #8813133 - Flags: review?(jwalden+bmo)
Attachment #8813135 - Flags: review?(jwalden+bmo)
Assignee

Comment 7

3 years ago
Posted patch trace-5-srcSplinter Review
Rename methods in the root directory and refactor the weak map trace method.
Attachment #8813146 - Flags: review?(jdemooij)
Assignee

Comment 8

3 years ago
Posted patch trace-6-gcSplinter Review
Rename methods in gc directory and add GCMarker::fromTracer().
Attachment #8813147 - Flags: review?(bbouvier)
Comment on attachment 8813147 [details] [diff] [review]
trace-6-gc

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

Approved, thank you for the patch.

::: js/src/gc/Barrier.h
@@ -249,5 @@
> -
> -// Marking.h depends on these barrier definitions, so we need a separate
> -// entry point for marking to implement the pre-barrier.
> -void MarkValueForBarrier(JSTracer* trc, Value* v, const char* name);
> -void MarkIdForBarrier(JSTracer* trc, jsid* idp, const char* name);

Good catch!

::: js/src/gc/Marking.cpp
@@ +664,5 @@
>      MOZ_ASSERT(trc->isCallbackTracer());
>      DoCallback(trc->asCallbackTracer(), thingp, name);
>  }
>  
>  

pre-existing nit: strange trailing space to remove here
Attachment #8813147 - Flags: review?(bbouvier) → review+
Attachment #8813119 - Flags: review?(jdemooij) → review+
Attachment #8813121 - Flags: review?(jdemooij) → review+
Comment on attachment 8813146 [details] [diff] [review]
trace-5-src

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

::: js/src/jsweakmap.h
@@ +53,5 @@
>  
>      // Unmark all weak maps in a zone.
>      static void unmarkZone(JS::Zone* zone);
>  
>      // Mark all the weakmaps in a zone.

Nit: s/Mark/Trace

@@ +206,2 @@
>              marked = true;
> +            (void) markIteratively(static_cast<GCMarker*>(trc));

Can |trc->weakMapAction() == DoNotTraceWeakMaps| be true in this case? If yes this behaves differently from how it did before (due to the if-statement below). Maybe assert?
Attachment #8813146 - Flags: review?(jdemooij) → review+
Comment on attachment 8813135 [details] [diff] [review]
trace-4-vm

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

::: js/src/jsgc.cpp
@@ +2551,2 @@
>  
>          WeakMapBase::markAll(zone, &trc);

Doesn't this need to be named traceAll as well?  (Maybe a different patch fixes this?)

::: js/src/vm/Debugger.cpp
@@ +2969,5 @@
>   * returns true if it has to mark anything; GC calls it repeatedly until it
>   * returns false.
>   */
>  /* static */ bool
> +Debugger::markIteratively(GCMarker* trc)

Should a "mark" function that takes a GCMarker* name that argument "trc"?  Seems like no to me.
Attachment #8813135 - Flags: review?(jwalden+bmo) → review+
Attachment #8813132 - Flags: review?(hv1989) → review+
Assignee

Comment 12

3 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)
> >          WeakMapBase::markAll(zone, &trc);
> 
> Doesn't this need to be named traceAll as well?  (Maybe a different patch
> fixes this?)

Yes, that's fixed in patch 5.

> Should a "mark" function that takes a GCMarker* name that argument "trc"? 
> Seems like no to me.

Fair point, I've updated all the patches.
Assignee

Comment 13

3 years ago
(In reply to Jan de Mooij [:jandem] from comment #10)
> Can |trc->weakMapAction() == DoNotTraceWeakMaps| be true in this case?

Good catch, but no it can't and this is asserted.

Comment 14

3 years ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a29a037a335
Standardise names of tracing methods r=jandem r=h4writer r=waldo r=bbouvier

Comment 15

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4a29a037a335
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.