Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1435643002:
Make gen-postmortem-metadata.py more reliable (Closed)
Description[tools] Make gen-postmortem-metadata.py more reliable
Instead of basing matches off of whitespace, walk the inheritance chain and include any classes that inherit from Object.
R=machenbach@chromium.org,jkummerow@chromium.org
NOTRY=true
Committed: https://crrev.com/bc2e393b4cc474a817c3fc5f9c5d7e58636f6ae5
Cr-Commit-Position: refs/heads/master@{#31964}
Patch Set 1 #
Total comments: 9
Patch Set 2 : #Messages
Total messages: 14 (3 generated)
An explicit comment is better than magic whitespace. However, it's completely non-obvious which classes get the annotation and which don't (see a few examples below). How about the following algorithm: look at every class definition initially; if you can trace its inheritance chain to HeapObject, process it further; otherwise ignore it. That should give you exactly what you need for heap analysis, and would be maintainance free as classes get added and removed. You wouldn't need any annotations in objects.h. https://codereview.chromium.org/1435643002/diff/1/src/objects.h File src/objects.h (right): https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode1641 src/objects.h:1641: class BodyDescriptorBase { /* POSTMORTEM */ really? https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode2143 src/objects.h:2143: class PrototypeRegistryCompactionCallback { /* POSTMORTEM */ really? https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode2438 src/objects.h:2438: class SpillInformation { /* POSTMORTEM */ really? https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode2764 src/objects.h:2764: class NullCallback { /* POSTMORTEM */ really? https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode2779 src/objects.h:2779: class Iterator { /* POSTMORTEM */ really? https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode3017 src/objects.h:3017: class WhitenessWitness { /* POSTMORTEM */ really? https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode3027 src/objects.h:3027: class Entry { /* POSTMORTEM */ really? https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode3115 src/objects.h:3115: class BaseShape { /* POSTMORTEM */ really? https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode3298 src/objects.h:3298: class HashTableKey { /* POSTMORTEM */ really?
On 2015/11/10 15:12:12, Jakob wrote: > An explicit comment is better than magic whitespace. > > However, it's completely non-obvious which classes get the annotation and which > don't (see a few examples below). > > How about the following algorithm: look at every class definition initially; if > you can trace its inheritance chain to HeapObject, process it further; otherwise > ignore it. That should give you exactly what you need for heap analysis, and > would be maintainance free as classes get added and removed. You wouldn't need > any annotations in objects.h. > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h > File src/objects.h (right): > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode1641 > src/objects.h:1641: class BodyDescriptorBase { /* POSTMORTEM */ > really? > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode2143 > src/objects.h:2143: class PrototypeRegistryCompactionCallback { /* POSTMORTEM */ > really? > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode2438 > src/objects.h:2438: class SpillInformation { /* POSTMORTEM */ > really? > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode2764 > src/objects.h:2764: class NullCallback { /* POSTMORTEM */ > really? > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode2779 > src/objects.h:2779: class Iterator { /* POSTMORTEM */ > really? > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode3017 > src/objects.h:3017: class WhitenessWitness { /* POSTMORTEM */ > really? > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode3027 > src/objects.h:3027: class Entry { /* POSTMORTEM */ > really? > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode3115 > src/objects.h:3115: class BaseShape { /* POSTMORTEM */ > really? > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode3298 > src/objects.h:3298: class HashTableKey { /* POSTMORTEM */ > really? That's fair. I added everything that was already being caught via the previous gen-postmortem-metadate script, but I think your suggestion would definitely be better. I'll work on that. Thanks for the review!
On 2015/11/10 15:17:47, evan.lucas wrote: > On 2015/11/10 15:12:12, Jakob wrote: > > An explicit comment is better than magic whitespace. > > > > However, it's completely non-obvious which classes get the annotation and > which > > don't (see a few examples below). > > > > How about the following algorithm: look at every class definition initially; > if > > you can trace its inheritance chain to HeapObject, process it further; > otherwise > > ignore it. That should give you exactly what you need for heap analysis, and > > would be maintainance free as classes get added and removed. You wouldn't need > > any annotations in objects.h. > > > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h > > File src/objects.h (right): > > > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode1641 > > src/objects.h:1641: class BodyDescriptorBase { /* POSTMORTEM */ > > really? > > > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode2143 > > src/objects.h:2143: class PrototypeRegistryCompactionCallback { /* POSTMORTEM > */ > > really? > > > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode2438 > > src/objects.h:2438: class SpillInformation { /* POSTMORTEM */ > > really? > > > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode2764 > > src/objects.h:2764: class NullCallback { /* POSTMORTEM */ > > really? > > > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode2779 > > src/objects.h:2779: class Iterator { /* POSTMORTEM */ > > really? > > > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode3017 > > src/objects.h:3017: class WhitenessWitness { /* POSTMORTEM */ > > really? > > > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode3027 > > src/objects.h:3027: class Entry { /* POSTMORTEM */ > > really? > > > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode3115 > > src/objects.h:3115: class BaseShape { /* POSTMORTEM */ > > really? > > > > https://codereview.chromium.org/1435643002/diff/1/src/objects.h#newcode3298 > > src/objects.h:3298: class HashTableKey { /* POSTMORTEM */ > > really? > > That's fair. I added everything that was already being caught via the previous > gen-postmortem-metadate script, but I think your suggestion would definitely be > better. I'll work on that. > > Thanks for the review! Instead of ignoring everything not from HeapObject, should we not instead ignore everything not from Object? I know some of the postmortem tools for Node.js use Smi which inherits from Object
> Instead of ignoring everything not from HeapObject, should we not instead ignore > everything not from Object? I know some of the postmortem tools for Node.js use > Smi which inherits from Object Sure, whatever makes sense.
On 2015/11/10 15:29:47, Jakob wrote: > > Instead of ignoring everything not from HeapObject, should we not instead > ignore > > everything not from Object? I know some of the postmortem tools for Node.js > use > > Smi which inherits from Object > > Sure, whatever makes sense. Ok, I updated gen-postmortem-metadata.py to walk the inheritance chain in Patch Set 2. PTAL. Thanks!
Yes, much better. The issue description needs updating. gen-postmortem-metadata.py's syntax isn't very pythonic, but at least the new bits are consistent with the rest of it, and I don't really care, so... LGTM.
Description was changed from ========== Make gen-postmortem-metadata.py more reliable Look for marker /* POSTMORTEM */ in objects.h instead of basing matches off of whitespace for generating postmortem metadata. This requires explicitly marking classes for inclusion in the metadata. Replaces https://codereview.chromium.org/1420693005/ R=machenbach@chromium.org,jkummerow@chromium.org BUG= ========== to ========== Make gen-postmortem-metadata.py more reliable Instead of basing matches off of whitespace, walk the inheritance chain and include any classes that inherit from Object. Replaces https://codereview.chromium.org/1420693005/ R=machenbach@chromium.org,jkummerow@chromium.org BUG= ==========
On 2015/11/11 09:15:55, Jakob wrote: > Yes, much better. > > The issue description needs updating. > > gen-postmortem-metadata.py's syntax isn't very pythonic, but at least the new > bits are consistent with the rest of it, and I don't really care, so... LGTM. Issue description updated. Thanks!
Description was changed from
==========
Make gen-postmortem-metadata.py more reliable
Instead of basing matches off of whitespace, walk the inheritance chain and
include any classes that inherit from Object.
Replaces https://codereview.chromium.org/1420693005/
R=machenbach@chromium.org,jkummerow@chromium.org
BUG=
==========
to
==========
[tools] Make gen-postmortem-metadata.py more reliable
Instead of basing matches off of whitespace, walk the inheritance chain and
include any classes that inherit from Object.
R=machenbach@chromium.org,jkummerow@chromium.org
NOTRY=true
==========
The CQ bit was checked by jkummerow@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1435643002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1435643002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/bc2e393b4cc474a817c3fc5f9c5d7e58636f6ae5 Cr-Commit-Position: refs/heads/master@{#31964} |
