From d17e4f04d6190faebc756b2816d7c10d8ce81f20 Mon Sep 17 00:00:00 2001 From: Matt Blagden Date: Tue, 9 Jul 2024 11:38:20 -0700 Subject: [PATCH] Separate getPropertyNames return record Summary: Original Author: mattblagden@meta.com Original Git: b192d6307c8db01f9a38fcc1e9f3d893ccf7c80d Original Reviewed By: neildhar Original Revision: D59113879 Emit separate records for the invocation of getPropertyNames and its return value, allowing other records to exist in between. Keep the return value on the getPropertyNames record for now so old traces can be used, but don't emit it in any new traces. Reviewed By: neildhar Differential Revision: D59403433 fbshipit-source-id: 092764bd01c14e842e24123ed2e6d0424203672c --- API/hermes/SynthTrace.cpp | 1 - API/hermes/SynthTrace.h | 10 +++- API/hermes/SynthTraceParser.cpp | 12 ++-- API/hermes/TraceInterpreter.cpp | 7 ++- API/hermes/TracingRuntime.cpp | 6 +- unittests/API/SynthTraceSerializationTest.cpp | 4 +- unittests/API/SynthTraceTest.cpp | 58 ++++++++++++++++--- 7 files changed, 77 insertions(+), 21 deletions(-) diff --git a/API/hermes/SynthTrace.cpp b/API/hermes/SynthTrace.cpp index 1f3bcfbb94c..7dba9767e9e 100644 --- a/API/hermes/SynthTrace.cpp +++ b/API/hermes/SynthTrace.cpp @@ -430,7 +430,6 @@ void SynthTrace::GetPropertyNamesRecord::toJSONInternal( JSONEmitter &json) const { Record::toJSONInternal(json); json.emitKeyValue("objID", objID_); - json.emitKeyValue("propNamesID", propNamesID_); } void SynthTrace::CreateArrayRecord::toJSONInternal(JSONEmitter &json) const { diff --git a/API/hermes/SynthTrace.h b/API/hermes/SynthTrace.h index cd3538bab20..46110d7e011 100644 --- a/API/hermes/SynthTrace.h +++ b/API/hermes/SynthTrace.h @@ -882,12 +882,13 @@ class SynthTrace { // Since getPropertyNames always returns an array, this can be an object id // rather than a TraceValue. /// The ObjectID of the array that was returned by getPropertyNames(). - const ObjectID propNamesID_; + // TODO: delete once no traces contain this field + const std::optional propNamesID_; explicit GetPropertyNamesRecord( TimeSinceStart time, ObjectID objID, - ObjectID propNamesID) + std::optional propNamesID = std::nullopt) : Record(time), objID_(objID), propNamesID_(propNamesID) {} void toJSONInternal(::hermes::JSONEmitter &json) const override; @@ -895,7 +896,10 @@ class SynthTrace { return type; } std::vector defs() const override { - return {propNamesID_}; + if (!propNamesID_) { + return {}; + } + return {*propNamesID_}; } std::vector uses() const override { return {objID_}; diff --git a/API/hermes/SynthTraceParser.cpp b/API/hermes/SynthTraceParser.cpp index 5de1707d75d..5c4f32e17d6 100644 --- a/API/hermes/SynthTraceParser.cpp +++ b/API/hermes/SynthTraceParser.cpp @@ -434,12 +434,16 @@ SynthTrace getTrace( #endif ); break; - case RecordType::GetPropertyNames: + case RecordType::GetPropertyNames: { + JSONValue *propNamesIDVal = obj->get("propNamesID"); + std::optional propNamesId; + if (propNamesIDVal) { + propNamesId = getNumberAs(propNamesIDVal); + } trace.emplace_back( - timeFromStart, - objID->getValue(), - getNumberAs(obj->get("propNamesID"))); + timeFromStart, objID->getValue(), propNamesId); break; + } case RecordType::CreateArray: trace.emplace_back( timeFromStart, diff --git a/API/hermes/TraceInterpreter.cpp b/API/hermes/TraceInterpreter.cpp index 8883ee6e609..801afa03648 100644 --- a/API/hermes/TraceInterpreter.cpp +++ b/API/hermes/TraceInterpreter.cpp @@ -912,7 +912,12 @@ void TraceInterpreter::executeRecords() { jsi::Array arr = getJSIValueForUse(gpnr.objID_) .asObject(rt_) .getPropertyNames(rt_); - addToObjectMap(gpnr.propNamesID_, std::move(arr), currentExecIndex); + if (gpnr.propNamesID_) { + addToObjectMap( + *gpnr.propNamesID_, std::move(arr), currentExecIndex); + } else { + retval = std::move(arr); + } break; } case RecordType::CreateArray: { diff --git a/API/hermes/TracingRuntime.cpp b/API/hermes/TracingRuntime.cpp index 02436c67584..12ce0b37c7f 100644 --- a/API/hermes/TracingRuntime.cpp +++ b/API/hermes/TracingRuntime.cpp @@ -627,9 +627,11 @@ void TracingRuntime::setPropertyValue( } jsi::Array TracingRuntime::getPropertyNames(const jsi::Object &o) { - jsi::Array arr = RD::getPropertyNames(o); trace_.emplace_back( - getTimeSinceStart(), useObjectID(o), defObjectID(arr)); + getTimeSinceStart(), useObjectID(o)); + jsi::Array arr = RD::getPropertyNames(o); + trace_.emplace_back( + getTimeSinceStart(), SynthTrace::encodeObject(defObjectID(arr))); return arr; } diff --git a/unittests/API/SynthTraceSerializationTest.cpp b/unittests/API/SynthTraceSerializationTest.cpp index bf5a54cae23..51ae333d01c 100644 --- a/unittests/API/SynthTraceSerializationTest.cpp +++ b/unittests/API/SynthTraceSerializationTest.cpp @@ -188,8 +188,8 @@ TEST_F(SynthTraceSerializationTest, HasProperty) { TEST_F(SynthTraceSerializationTest, GetPropertyNames) { EXPECT_EQ( - R"({"type":"GetPropertyNamesRecord","time":0,"objID":1,"propNamesID":2})", - to_string(SynthTrace::GetPropertyNamesRecord(dummyTime, 1, 2))); + R"({"type":"GetPropertyNamesRecord","time":0,"objID":1})", + to_string(SynthTrace::GetPropertyNamesRecord(dummyTime, 1))); } TEST_F(SynthTraceSerializationTest, CreateArray) { diff --git a/unittests/API/SynthTraceTest.cpp b/unittests/API/SynthTraceTest.cpp index b1e86a33f50..371627797db 100644 --- a/unittests/API/SynthTraceTest.cpp +++ b/unittests/API/SynthTraceTest.cpp @@ -483,12 +483,16 @@ TEST_F(SynthTraceTest, GetPropertyNames) { propNamesID = rt->useObjectID(names); } const auto &records = rt->trace().records(); - EXPECT_EQ(2, records.size()); + EXPECT_EQ(3, records.size()); EXPECT_EQ_RECORD( SynthTrace::CreateObjectRecord(records[0]->time_, objID), *records[0]); EXPECT_EQ_RECORD( - SynthTrace::GetPropertyNamesRecord(records[1]->time_, objID, propNamesID), + SynthTrace::GetPropertyNamesRecord(records[1]->time_, objID), *records[1]); + EXPECT_EQ_RECORD( + SynthTrace::ReturnToNativeRecord( + records[2]->time_, SynthTrace::encodeObject(propNamesID)), + *records[2]); } TEST_F(SynthTraceTest, CreateArray) { @@ -1231,7 +1235,7 @@ struct SynthTraceRuntimeTest : public ::testing::Test { std::make_unique(traceResult), /* forReplay */ false)) {} - jsi::Value eval(jsi::Runtime &rt, std::string code) { + static jsi::Value eval(jsi::Runtime &rt, std::string code) { return rt.global().getPropertyAsFunction(rt, "eval").call(rt, code); } }; @@ -1924,11 +1928,10 @@ TEST_F(SynthTraceReplayTest, HostFunctionAsGetter) { rt, jsi::PropNameID::forAscii(rt, "foo"), 0, - [this]( - jsi::Runtime &rt, - const jsi::Value &thisVal, - const jsi::Value *args, - size_t count) { return eval(rt, "++flag"); }); + [](jsi::Runtime &rt, + const jsi::Value &thisVal, + const jsi::Value *args, + size_t count) { return eval(rt, "++flag"); }); // Attach the host function to the property getter of an object auto obj = jsi::Object(rt); @@ -1956,6 +1959,45 @@ TEST_F(SynthTraceReplayTest, HostFunctionAsGetter) { } } +// This test exercises the case where getting property names triggers +// other records to be emitted before returning the property names. +TEST_F(SynthTraceReplayTest, HostFunctionAsGetPropertyNames) { + // Host object to carry out some actions that generate records when property + // names are requested. + class TestHostObject : public jsi::HostObject { + public: + TestHostObject() {} + + std::vector getPropertyNames(jsi::Runtime &rt) override { + SynthTraceRuntimeTest::eval(rt, "++flag"); + return std::vector{}; + } + }; + + { + auto &rt = *traceRt; + + // Setup a flag to ensure the host object's getPropertyNames() was invoked. + rt.global().setProperty(rt, "flag", 0); + + // Get the property names. + auto obj = jsi::Object::createFromHostObject( + rt, std::make_shared()); + rt.getPropertyNames(obj); + rt.getPropertyNames(obj); + } + + // Ensure the records were emitted in the order that correctly represents + // the actual execution. + replay(); + + // Ensure the host object's getPropertyNames() was invoked twice. + { + auto &rt = *replayRt; + EXPECT_EQ(eval(rt, "flag").getNumber(), 2); + } +} + using JobQueueReplayTest = SynthTraceReplayTest; TEST_F(JobQueueReplayTest, DrainSingleMicrotask) {