Skip to content

Commit bcb5080

Browse files
Riley Dulinfacebook-github-bot
Riley Dulin
authored and
facebook-github-bot
committed
Add symbol nodes from the IdentifierTable to heap snapshots
Summary: I realized that symbols were not being included in the heap snapshot output. This includes both internal symbols and user-created symbols. Since all symbols are anchored by the IdentifierTable, use it to create the nodes. The rest of the heap marks symbol edges as normal. Reviewed By: neildhar Differential Revision: D25315819 fbshipit-source-id: 7699c4e8f692f95a161e2f5577876d61a90b7b84
1 parent d483254 commit bcb5080

9 files changed

Lines changed: 53 additions & 42 deletions

File tree

‎include/hermes/VM/GCBase.h‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,8 @@ class GCBase {
238238
/// as it is slow. The function passed as acceptor shouldn't perform any
239239
/// heap operations.
240240
virtual void visitIdentifiers(
241-
const std::function<void(UTF16Ref, uint32_t id)> &acceptor) = 0;
241+
const std::function<void(SymbolID, const StringPrimitive *)>
242+
&acceptor) = 0;
242243

243244
/// Convert the given symbol into its UTF-8 string representation.
244245
/// \post The implementation of this function must not perform any GC
@@ -522,9 +523,9 @@ class GCBase {
522523
/// ID.
523524
inline HeapSnapshot::NodeID getObjectIDMustExist(const void *cell);
524525

525-
/// Get the unique object id of the symbol with the given index \b
526-
/// symIdx. If one does not yet exist, start tracking it.
527-
inline HeapSnapshot::NodeID getObjectID(uint32_t symIdx);
526+
/// Get the unique object id of the symbol with the given symbol \p sym. If
527+
/// one does not yet exist, start tracking it.
528+
inline HeapSnapshot::NodeID getObjectID(SymbolID sym);
528529

529530
/// Get the unique object id of the given native memory (non-JS-heap).
530531
/// If one does not yet exist, start tracking it.
@@ -991,7 +992,7 @@ class GCBase {
991992

992993
inline HeapSnapshot::NodeID getObjectID(const void *cell);
993994
inline HeapSnapshot::NodeID getObjectID(const GCPointerBase &cell);
994-
inline HeapSnapshot::NodeID getObjectID(const SymbolID &sym);
995+
inline HeapSnapshot::NodeID getObjectID(SymbolID sym);
995996
inline HeapSnapshot::NodeID getNativeID(const void *mem);
996997
/// \return The ID for the given value. If the value cannot be represented
997998
/// with an ID, returns None.
@@ -1566,8 +1567,8 @@ inline HeapSnapshot::NodeID GCBase::getObjectID(const GCPointerBase &cell) {
15661567
return getObjectID(cell.get(pointerBase_));
15671568
}
15681569

1569-
inline HeapSnapshot::NodeID GCBase::getObjectID(const SymbolID &sym) {
1570-
return idTracker_.getObjectID(sym.unsafeGetIndex());
1570+
inline HeapSnapshot::NodeID GCBase::getObjectID(SymbolID sym) {
1571+
return idTracker_.getObjectID(sym);
15711572
}
15721573

15731574
inline HeapSnapshot::NodeID GCBase::getNativeID(const void *mem) {
@@ -1605,15 +1606,15 @@ inline HeapSnapshot::NodeID GCBase::IDTracker::getObjectIDMustExist(
16051606
return iter->second;
16061607
}
16071608

1608-
inline HeapSnapshot::NodeID GCBase::IDTracker::getObjectID(uint32_t symIdx) {
1609+
inline HeapSnapshot::NodeID GCBase::IDTracker::getObjectID(SymbolID sym) {
16091610
std::lock_guard<Mutex> lk{mtx_};
1610-
auto iter = symbolIDMap_.find(symIdx);
1611+
auto iter = symbolIDMap_.find(sym.unsafeGetIndex());
16111612
if (iter != symbolIDMap_.end()) {
16121613
return iter->second;
16131614
}
16141615
// Else, assume it is a symbol that needs to be tracked and give it a new ID.
16151616
const auto symID = nextObjectID();
1616-
symbolIDMap_[symIdx] = symID;
1617+
symbolIDMap_[sym.unsafeGetIndex()] = symID;
16171618
return symID;
16181619
}
16191620

‎include/hermes/VM/IdentifierTable.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,8 @@ class IdentifierTable {
153153
/// the entry and its id as arguments. This is intended to be used only for
154154
/// snapshots, as it is slow. The function passed as acceptor shouldn't
155155
/// perform any heap operations
156-
void visitIdentifiers(const std::function<void(UTF16Ref, uint32_t)> &lambda);
156+
void visitIdentifiers(
157+
const std::function<void(SymbolID, const StringPrimitive *)> &lambda);
157158

158159
/// \return one higher than the largest symbol in the identifier table. This
159160
/// enables the GC to size its internal structures for symbol marking.

‎include/hermes/VM/Runtime.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -877,7 +877,8 @@ class Runtime : public HandleRootOwner,
877877
/// snapshots, as it is slow. The function passed as acceptor shouldn't
878878
/// perform any heap operations.
879879
void visitIdentifiers(
880-
const std::function<void(UTF16Ref, uint32_t id)> &acceptor) override;
880+
const std::function<void(SymbolID, const StringPrimitive *)> &acceptor)
881+
override;
881882

882883
#ifdef HERMESVM_PROFILER_BB
883884
public:

‎lib/VM/GCBase.cpp‎

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -259,10 +259,7 @@ struct EdgeAddingAcceptor : public SnapshotAcceptor, public WeakRefAcceptor {
259259
}
260260

261261
void acceptHV(HermesValue &hv, const char *name) override {
262-
if (hv.isPointer()) {
263-
auto ptr = hv.getPointer();
264-
accept(ptr, name);
265-
} else if (auto id = gc.getSnapshotID(hv)) {
262+
if (auto id = gc.getSnapshotID(hv)) {
266263
snap_.addNamedEdge(
267264
HeapSnapshot::EdgeType::Internal,
268265
llvh::StringRef::withNullAsEmpty(name),
@@ -289,6 +286,16 @@ struct EdgeAddingAcceptor : public SnapshotAcceptor, public WeakRefAcceptor {
289286
gc.getObjectID(slot->getPointer()));
290287
}
291288

289+
void accept(SymbolID sym, const char *name) override {
290+
if (sym.isInvalid()) {
291+
return;
292+
}
293+
snap_.addNamedEdge(
294+
HeapSnapshot::EdgeType::Internal,
295+
llvh::StringRef::withNullAsEmpty(name),
296+
gc.getObjectID(sym));
297+
}
298+
292299
private:
293300
// For unnamed edges, use indices instead.
294301
unsigned nextEdge_{0};
@@ -433,7 +440,7 @@ void GCBase::createSnapshot(GC *gc, llvh::raw_ostream &os) {
433440
JSONEmitter json(os);
434441
HeapSnapshot snap(json, gcCallbacks_->getStackTracesTree());
435442

436-
const auto rootScan = [gc, &snap]() {
443+
const auto rootScan = [gc, &snap, this]() {
437444
{
438445
// Make the super root node and add edges to each root section.
439446
SnapshotRootSectionAcceptor rootSectionAcceptor(*gc, snap);
@@ -471,6 +478,22 @@ void GCBase::createSnapshot(GC *gc, llvh::raw_ostream &os) {
471478
gc->markRoots(rootAcceptor, true);
472479
gc->markWeakRoots(rootAcceptor);
473480
}
481+
gcCallbacks_->visitIdentifiers(
482+
[gc, &snap, this](SymbolID sym, const StringPrimitive *str) {
483+
snap.beginNode();
484+
if (str) {
485+
snap.addNamedEdge(
486+
HeapSnapshot::EdgeType::Internal,
487+
"description",
488+
gc->getObjectID(str));
489+
}
490+
snap.endNode(
491+
HeapSnapshot::NodeType::Symbol,
492+
gc->convertSymbolToUTF8(sym),
493+
idTracker_.getObjectID(sym),
494+
sizeof(SymbolID),
495+
0);
496+
});
474497
};
475498

476499
snap.beginSection(HeapSnapshot::Section::Nodes);
@@ -1100,6 +1123,8 @@ llvh::Optional<HeapSnapshot::NodeID> GCBase::getSnapshotID(HermesValue val) {
11001123
: IDTracker::reserved(IDTracker::ReservedObjectID::Null);
11011124
} else if (val.isNumber()) {
11021125
return idTracker_.getNumberID(val.getNumber());
1126+
} else if (val.isSymbol() && val.getSymbol().isValid()) {
1127+
return idTracker_.getObjectID(val.getSymbol());
11031128
} else if (val.isUndefined()) {
11041129
return IDTracker::reserved(IDTracker::ReservedObjectID::Undefined);
11051130
} else if (val.isNull()) {

‎lib/VM/HiddenClass.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ const VTable HiddenClass::vt{
108108

109109
void HiddenClassBuildMeta(const GCCell *cell, Metadata::Builder &mb) {
110110
const auto *self = static_cast<const HiddenClass *>(cell);
111-
mb.addField(&self->symbolID_);
111+
mb.addField("symbol", &self->symbolID_);
112112
mb.addField("parent", &self->parent_);
113113
mb.addField("propertyMap", &self->propertyMap_);
114114
mb.addField("forInCache", &self->forInCache_);

‎lib/VM/IdentifierTable.cpp‎

Lines changed: 5 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -247,28 +247,14 @@ void IdentifierTable::markIdentifiers(RootAcceptor &acceptor, GC *gc) {
247247
}
248248

249249
void IdentifierTable::visitIdentifiers(
250-
const std::function<void(UTF16Ref, uint32_t)> &acceptor) {
250+
const std::function<void(SymbolID, const StringPrimitive *)> &acceptor) {
251251
for (uint32_t i = 0; i < getSymbolsEnd(); ++i) {
252252
auto &vectorEntry = getLookupTableEntry(i);
253-
vm::SmallU16String<16> allocator;
254-
UTF16Ref ref;
255-
if (vectorEntry.isStringPrim()) {
256-
allocator.clear();
257-
auto stringPrim = vectorEntry.getStringPrim();
258-
stringPrim->appendUTF16String(allocator);
259-
ref = allocator.arrayRef();
260-
} else if (vectorEntry.isLazyASCII()) {
261-
allocator.clear();
262-
auto asciiRef = vectorEntry.getLazyASCIIRef();
263-
std::copy(
264-
asciiRef.begin(), asciiRef.end(), std::back_inserter(allocator));
265-
ref = allocator.arrayRef();
266-
} else if (vectorEntry.isLazyUTF16()) {
267-
ref = vectorEntry.getLazyUTF16Ref();
268-
} else {
269-
continue;
253+
if (!vectorEntry.isFreeSlot()) {
254+
const StringPrimitive *str =
255+
vectorEntry.isStringPrim() ? vectorEntry.getStringPrim() : nullptr;
256+
acceptor(SymbolID::unsafeCreate(i), str);
270257
}
271-
acceptor(ref, i);
272258
}
273259
}
274260

‎lib/VM/Runtime.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -553,7 +553,7 @@ void Runtime::markWeakRoots(WeakRootAcceptor &acceptor) {
553553
}
554554

555555
void Runtime::visitIdentifiers(
556-
const std::function<void(UTF16Ref, uint32_t)> &acceptor) {
556+
const std::function<void(SymbolID, const StringPrimitive *)> &acceptor) {
557557
identifierTable_.visitIdentifiers(acceptor);
558558
}
559559

‎unittests/VMRuntime/TestHelpers.cpp‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,6 @@ void DummyRuntime::markWeakRoots(WeakRootAcceptor &acceptor) {
7777
}
7878

7979
std::string DummyRuntime::convertSymbolToUTF8(SymbolID) {
80-
assert(false && "Should never attempt to resolve a symbol on a DummyRuntime");
8180
return "";
8281
}
8382

‎unittests/VMRuntime/TestHelpers.h‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -354,9 +354,7 @@ struct DummyRuntime final : public HandleRootOwner,
354354
void printRuntimeGCStats(JSONEmitter &) const override {}
355355

356356
void visitIdentifiers(
357-
const std::function<void(UTF16Ref, uint32_t)> &acceptor) override {
358-
acceptor(createUTF16Ref(u"DummyIdTableEntry0"), 0);
359-
acceptor(createUTF16Ref(u"DummyIdTableEntry1"), 1);
357+
const std::function<void(SymbolID, const StringPrimitive *)> &) override {
360358
}
361359

362360
std::string convertSymbolToUTF8(SymbolID) override;

0 commit comments

Comments
 (0)