[tsapeta]

Summary

React Native reads every props object with Object::getPropertyNames, then getValueAtIndex, getString, getProperty(String) and utf8 for each name: about four JSI calls, three pinned values and two name conversions per property.

This adds a JSI interface that does the whole walk in one call:

class IPropertyEnumeration : public ICast {
 public:
  virtual void forEachProperty(
      const Object& object,
      void* ctx,
      void (*cb)(
          void* ctx,
          bool ascii,
          const void* name,
          size_t length,
          Value& value)) = 0;
};

The result is the same as calling getProperty for each name of getPropertyNames. The doc comment defines the details: the names are collected before the first call, getters and Proxy traps run, what happens to properties that are added or deleted during the enumeration, and how exceptions from getters and from the callback stop it.

• It's a separate, optional interface that callers get with castInterface, so IRuntime and its ABI don't change. A runtime without it returns nullptr, and the caller uses getPropertyNames. TracingRuntime returns nullptr for it too, so a trace records the calls of that fallback.
• It isn't behind JSI_UNSTABLE, so React Native can use it as soon as a Hermes release has it. Because it's optional, freezing it is cheap: if the design has to change after a release, a runtime can stop returning this interface and provide a new one, and callers fall back to getPropertyNames in the meantime. I can move it behind JSI_UNSTABLE if you prefer.
• The name comes like the data of getStringData: an ascii flag, then char or char16_t units. JSI doesn't convert names to UTF-8, and every name comes through unchanged, including unpaired surrogates.
• The callback may use the runtime, which it needs to read the values. So, unlike getStringData, Hermes copies each name into a buffer that it reuses, and the name is valid only during the call.
• Hermes reads the for-in names, which are symbols for ordinary objects, and gets each value with getNamedOrIndexed, the same way as getProperty(PropNameID). There's no names array and no jsi::String per name.

RuntimeDecorator and WithRuntimeDecorator forward castInterface to the plain runtime, so calls through the interface of the thread-safe runtime don't take its lock. That's the same for ISerialization today.

Test Plan

• New tests in unittests/API/APITest.cpp, for makeHermesRuntime and makeThreadSafeHermesRuntime:
  • ForEachPropertyMatchesGetProperty: for objects with string, index, non-ASCII, unpaired surrogate, symbol, inherited, getter and non-enumerable properties, an array, a Proxy and an empty object, the names and values are the same as with getPropertyNames + getProperty. The callback allocates and runs a garbage collection, and the name stays valid.
  • ForEachPropertyWhenTheObjectChanges: a property that the callback deletes is still visited with undefined, and a property that it adds isn't visited.
  • ForEachPropertyStopsOnExceptions: a getter that throws stops the enumeration with a JSError, an exception from the callback stops it and propagates, and the runtime stays usable.
• APITests: all 446 tests pass without JSI_UNSTABLE and all 517 with it. synthTrace and traceInterpreter build in both configurations.
• A timing loop on top of APITests (macOS host, Release, makeHermesRuntime, median of 9 samples of 200,000 iterations, not part of this PR) for the walk React Native does: names, values and name lengths, on objects with number-valued props.

Walk	getPropertyNames + strings	forEachProperty
1 prop	98 ns	29 ns
6 props	468 ns	130 ns
20 props	1,517 ns	437 ns
20 indices (array)	3,660 ns	986 ns

[tmikov]

This breaks the ABI. We need to discuss whether that is worthwhile, compared to adding a new interface.

The doc-comment on the new function is not great. It says "he callback must take in four arguments...", which is redundant, but does not explain any restrictions on the callback - can it allocate, can it modify the object that is being enumerated, can it throw, etc. There is no explanation of what happens with property names that are not convertable to UTF-8 either,

Additionally, converting to UTF-8 really is not JSI-s job, especially given that not everything can be converted.

For all of these reasons, I think this needs a careful redesign.

[tsapeta]

Thanks for the review, I agree with all three points. I've updated the PR:

ABI. forEachProperty is now in a separate, optional interface, IPropertyEnumeration, that callers get with castInterface. IRuntime doesn't change. A runtime that doesn't provide the interface returns nullptr, and the caller uses getPropertyNames as before. TracingRuntime returns nullptr for it too, so a trace records the calls of that fallback.

I didn't put it behind JSI_UNSTABLE, because I'm not sure what the process is, especially for features that React Native needs. I'm happy to move it behind the flag if that's the right way.

One thing I noticed: RuntimeDecorator and WithRuntimeDecorator forward castInterface to the plain runtime, so calls through an interface of the thread-safe runtime don't take its lock. That's the same for ISerialization today. I can look at it in a separate PR if you think it's worth it.

UTF-8. The conversion is gone. The callback gets the name the way getStringData gives the data: an ascii flag, and then char or char16_t units. So every name, including unpaired surrogates, comes through unchanged. Unlike getStringData, the callback may use the runtime, which it needs to do to read the values. So Hermes copies each name into a buffer that it reuses, and the name is valid only during the call.

Doc comment. I rewrote it. It now defines the behavior as the same as calling getProperty for each name of getPropertyNames:

• The names are collected before the first call.
• Getters and Proxy traps run.
• A property that is added during the enumeration isn't visited. A property that is deleted before it's visited is still visited, with the value that getProperty returns at that time.
• The callback may use the runtime, for example to allocate, call functions or modify the object.
• If a getter or Proxy trap throws, the enumeration stops and the method throws a JSError. If the callback throws, the enumeration stops and the exception propagates.

The new APITests cover each of these.