Summary: | Rename checkMarkByte() to checkIfEitherInEdenGenOrAlreadyRemembered() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | ||||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Mark Lam
2014-11-04 13:36:47 PST
Created attachment 240945 [details]
the patch.
Comment on attachment 240945 [details] the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=240945&action=review > Source/JavaScriptCore/ChangeLog:14 > + 1. If the cell is in the young generation. It has no old to eden pointers by > + definition. Remove the leading "If", since you prefixed the ":" with "if". > Source/JavaScriptCore/ChangeLog:15 > + 2. If the cell is already in the remembered set. Remove the leading "If". The logic of this paragraph is backwards. You either need to invert the numbered items or invert the statement before the colon. You need to clarify that #2 is just an optimization strategy, and not a strict requirement. It's totally fine to add the same object to a set twice -- it's just redundant, and we have an efficient way to avoid it. > Source/JavaScriptCore/ChangeLog:17 > + The barrier current names this check as checkMarkByte(). We should rename it to currently > Source/JavaScriptCore/ChangeLog:25 > + Similarly, Jump results of this check are currently named > + ownerNotMarkedOrAlreadyRemembered. This can be misinterpreted as the owner is > + not marked or not already remembered. We should rename it to > + ownerIsInEdenGenOrAlreadyRemembered which is clearer about the intent of the > + check. What we are really checking for is that the cell is in the eden gen, > + which is implied by being "not marked". It's a good point that the old name has an ambiguous modifier in it. But I think the new name has some ambiguity and verbosity too. > Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:251 > + AssemblyHelpers::Jump ownerIsInEdenGenOrAlreadyRemembered = jit.checkIfEitherInEdenGenOrAlreadyRemembered(owner); How about AssemblyHelpers::Jump ownerIsRememberedOrInEden = jit.jumpIfIsRememberedOrInEden(owner); > Source/JavaScriptCore/jit/AssemblyHelpers.h:668 > + // Note: gcData being NotMarked (a value of 1) implies that the cell is in the young GC generation. I don't understand this comment, and it seems out of place int he assembler. A better way to comment this is probably to list the set of possible bit patterns next to the declaration of GCData in JSCell.h. Created attachment 240969 [details]
take 2
Thanks for the feedback. I agree, and the suggested changes have been applied. I also change the LLINT macro name to skipIfIsRememberedOrInEden, which I think reads better than checkIfIsRememberedOrInEden or the original checkMarkByte. Comment on attachment 240969 [details] take 2 View in context: https://bugs.webkit.org/attachment.cgi?id=240969&action=review > Source/JavaScriptCore/runtime/JSCell.h:152 > + MarkedAndRemembered = 2, // The object is in the GC's remember set. remembered set > Source/JavaScriptCore/runtime/JSCell.h:154 > + // The object being in the GC's remember set implies that it is also remembered set > Source/JavaScriptCore/runtime/JSCell.h:155 > + // Marked. This is because objects are only added to the remembered sets remembered set (In reply to comment #5) > > Source/JavaScriptCore/runtime/JSCell.h:152 > > + MarkedAndRemembered = 2, // The object is in the GC's remember set. > > remembered set Oops. Thanks for the review. Fixed and landed in r175593: <http://trac.webkit.org/r175593>. |