Open Bug 1410695 Opened 7 years ago Updated 2 years ago

Consider adding lookup cache for some case in native code.

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

ASSIGNED
Tracking Status
firefox58 --- wontfix
firefox59 --- ?

People

(Reporter: arai, Assigned: arai)

References

(Blocks 2 open bugs)

Details

Attachments

(6 files, 1 obsolete file)

(derived from bug 1393712 comment #18) These days we do something like following in several places in native code, to detect optimizable case and enter optimized path after that. like, SpeciesConstructor's case. GetPropertyPure(cx, OBJ, ..., &value) if (SomeCondition(value)) { // optimizable } else { // not optimizable } there, OBJ is supposed to have same shape every time. we do LookupPropertyPure every time inside GetPropertyPure, but we could cache the shape and slot, and skip lookup.
Blocks: 1393712
For that matter, if there were a way to use ICs in places where native code calls GetProperty etc., we could speed *lots* of things up. Things like the interpreter.
Priority: -- → P3
based on bug 1317481. performance comparison for bug 1393712 testcase. before: ~440 [ms] after: ~400 [ms] (this will need to clear cache on GC, and also store cache per context or compartment tho...)
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
if we use cache per each callsite, the issue is where to store the cache. we could store them into context or compartment, but it will increase the size of them regardless of the usage.
so far I found 2 cases: 1. want to check if the given object has given property with same value 2. want to check if the given object never has given property (bug 1412206) I think, the case 2 can also be covered with the same way (checking shape for object and prototypes).
moved the cache from static variable to runtime
Attachment #8921429 - Attachment is obsolete: true
To make it easier to use this cache system in other place, it would be nice to integrate cache system into function like GetProperty function or similar, I'll check if it's possible to separate the conditions for each access in this case (currently the cache is updated at once when the whole conditions are met).
now checking how many times lookup happens in ARES6, and looks like we're calling functions, that does LookupOwnPropertyInline inside, more than 1000 times with same shape and same property id in some callsites, with same result (both found and not found cases). those will benefit from cache, as long as cache can build with faster way than shape. Here's the reverse call graph and the shape/id/found for each path, where it hit more than 1000 times. (it's partial execution of ARES-6 Air, not yet finished entire run) bool js::LookupOwnPropertyInline<(js::AllowGC)1>(JSContext*, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::PropertyResult, (js::AllowGC)1>::MutableHandleType, bool*) + bool NativeGetPropertyInline<(js::AllowGC)1>(JSContext*, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)1>::HandleType, IsNameLookup, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) | + js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) | + js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) | + js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, js::PropertyName*, JS::MutableHandle<JS::Value>) | + js::GetProperty(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) | + GetPropertyOperation(JSContext*, js::InterpreterFrame*, JS::Handle<JSScript*>, unsigned char*, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>) | + Interpret(JSContext*, js::RunState&) + 35118 | + js::RunScript(JSContext*, js::RunState&) | + shape=0x10641b560, id=args => not found: 8827 | + shape=0x106402200, id=push => not found: 14681 | + shape=0x10641b268, id=add => not found: 1228 + bool js::NativeLookupOwnProperty<(js::AllowGC)1>(JSContext*, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::PropertyResult, (js::AllowGC)1>::MutableHandleType) + js::NativeGetOwnPropertyDescriptor(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::PropertyDescriptor>) | + js::GetOwnPropertyDescriptor(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::PropertyDescriptor>) | + js::ScriptedProxyHandler::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) const | + js::Proxy::getInternal(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) | + js::ProxyGetPropertyByValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) | + 0x0 | + shape=0x106402200, id=[number] => found: 1181 + js::NativeDefineProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<jsid>, JS::Handle<JS::PropertyDescriptor>, JS::ObjectOpResult&) + js::NativeDefineDataProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, unsigned int, JS::ObjectOpResult&) | + js::NativeDefineDataProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, unsigned int) | + js::frontend::BytecodeEmitter::emitPropertyList(js::frontend::ParseNode*, JS::MutableHandle<js::PlainObject*>, js::frontend::PropListType) | + js::frontend::BytecodeEmitter::emitObject(js::frontend::ParseNode*) | + js::frontend::BytecodeEmitter::emitTree(js::frontend::ParseNode*, js::frontend::ValueUsage, js::frontend::BytecodeEmitter::EmitLineNumberNote) | + js::frontend::BytecodeEmitter::emitCallOrNew(js::frontend::ParseNode*, js::frontend::ValueUsage) | + shape=0x106406fb0, id=type => not found: 1302 | + shape=0x106404a10, id=role => not found: 1302 | + shape=0x106406fd8, id=width => not found: 1302 + js::DefineAccessorProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>), bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::ObjectOpResult&), unsigned int, JS::ObjectOpResult&) | + js::DefineAccessorProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>), bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::ObjectOpResult&), unsigned int) | + js::InitGetterSetterOperation(JSContext*, unsigned char*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JSObject*>) | + js::InitGetterSetterOperation(JSContext*, unsigned char*, JS::Handle<JSObject*>, JS::Handle<js::PropertyName*>, JS::Handle<JSObject*>) | + 0x0 | + shape=0x105cf9d80, id=liveSet => not found: 1026 + js::DefineDataProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, unsigned int, JS::ObjectOpResult&) + js::DefineDataProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, unsigned int) + js::DefineDataProperty(JSContext*, JS::Handle<JSObject*>, js::PropertyName*, JS::Handle<JS::Value>, unsigned int) | + ResolveInterpretedFunctionPrototype(JSContext*, JS::Handle<JSFunction*>, JS::Handle<jsid>) | + fun_resolve(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool*) | + js::CallResolveOp(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::PropertyResult>, bool*) | + shape=0x105cc7d08, id=constructor => not found: 1048 + js::DefineDataElement(JSContext*, JS::Handle<JSObject*>, unsigned int, JS::Handle<JS::Value>, unsigned int) + js::InitArrayElemOperation(JSContext*, unsigned char*, JS::Handle<JSObject*>, unsigned int, JS::Handle<JS::Value>) + Interpret(JSContext*, js::RunState&) + 71707 + js::RunScript(JSContext*, js::RunState&) + shape=0x105cb3040, id=[number] => not found: 2383 bool js::LookupOwnPropertyInline<(js::AllowGC)0>(JSContext*, js::MaybeRooted<js::NativeObject*, (js::AllowGC)0>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)0>::HandleType, js::MaybeRooted<JS::PropertyResult, (js::AllowGC)0>::MutableHandleType, bool*) bool js::LookupPropertyInline<(js::AllowGC)0>(JSContext*, js::MaybeRooted<js::NativeObject*, (js::AllowGC)0>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)0>::HandleType, js::MaybeRooted<JSObject*, (js::AllowGC)0>::MutableHandleType, js::MaybeRooted<JS::PropertyResult, (js::AllowGC)0>::MutableHandleType) + js::LookupNameNoGC(JSContext*, js::PropertyName*, JSObject*, JSObject**, JSObject**, JS::PropertyResult*) + bool js::GetEnvironmentName<(js::GetNameMode)0>(JSContext*, JS::Handle<JSObject*>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) + GetNameOperation(JSContext*, js::InterpreterFrame*, unsigned char*, JS::MutableHandle<JS::Value>) + Interpret(JSContext*, js::RunState&) + 48564 + js::RunScript(JSContext*, js::RunState&) + js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + InternalCall(JSContext*, js::AnyInvokeArgs const&) + shape=0x105cf9858, id=Set => not found: 1054 + shape=0x105cc7060, id=Set => not found: 1054 + shape=0x105ce09e8, id=Set => not found: 1054
here's the entire result for ARES-6, where lookup happens for same shape and same property, and same result, over 1000 times. I'm going to prototype cache system based on this result.
Attached patch (testing patch) Log lookup (deleted) — Splinter Review
fwiw, here's the patch I used to log lookup
Attached file (testing tool) parse_result.py (deleted) —
and script to parse the result
apparently, some functions are not shown in the result, because it's inlined :P
applied lookup cache to OrdinaryToPrimitive (it stores the cache to static tho). here's the performance comparison of ARES-6 ML [before] firstIteration: 284.90 +- 2.85 ms averageWorstCase: 239.93 +- 2.76 ms steadyState: 226.02 +- 2.36 ms summary: 249.06 +- 2.38 ms [after] firstIteration: 283.00 +- 3.90 ms averageWorstCase: 238.65 +- 2.70 ms steadyState: 224.07 +- 1.86 ms summary: 247.34 +- 2.04 ms
Interestingly, just adding lookup cache without info about callsite context improves performance :3 [before] firstIteration: 285.10 +- 7.63 ms averageWorstCase: 240.10 +- 5.32 ms steadyState: 224.08 +- 2.38 ms summary: 248.39 +- 2.34 ms [after] firstIteration: 278.60 +- 5.90 ms averageWorstCase: 234.68 +- 4.31 ms steadyState: 221.72 +- 3.48 ms summary: 243.82 +- 4.28 ms
in above cases, I'm mainly focusing on the following case: 1. object is an instance of builtin class 2. it's accessing prototype property 3. the direct prototype (obj->staticPrototype()) of the object has the property that way, it can skip lookup on the object itself and prototype of it.
result with all tests (above were running only ML) | before | after --------+------------------+--------------------+-------------------- Air | firstIteration | 101.83 +- 2.24 ms | 102.83 +- 1.68 ms | averageWorstCase | 60.13 +- 2.44 ms | 64.08 +- 7.46 ms | steadyState | 18.80 +- 0.15 ms | 18.98 +- 0.34 ms --------+------------------+--------------------+-------------------- Basic | firstIteration | 81.67 +- 1.58 ms | 83.00 +- 2.30 ms | averageWorstCase | 71.42 +- 8.37 ms | 67.25 +- 6.83 ms | steadyState | 52.81 +- 0.32 ms | 52.34 +- 0.30 ms --------+------------------+--------------------+-------------------- Babylon | firstIteration | 116.33 +- 4.72 ms | 119.00 +- 6.33 ms | averageWorstCase | 34.25 +- 2.39 ms | 33.21 +- 0.87 ms | steadyState | 7.60 +- 0.13 ms | 7.67 +- 0.19 ms --------+------------------+--------------------+-------------------- ML | firstIteration | 293.83 +- 17.50 ms | 288.67 +- 3.91 ms | averageWorstCase | 250.92 +- 15.89 ms | 238.46 +- 4.12 ms | steadyState | 225.95 +- 7.98 ms | 223.55 +- 3.70 ms --------+------------------+--------------------+-------------------- summary | 71.44 +- 1.37 ms | 71.16 +- 0.94 ms I'll check other places that lookup cache can be used.
I forgot that in most case the lookup fails. the length of prototype chain | not found ------------------------------+----------- 0 | 13229 1 | 46536 2 | 26411 3 | 220 4 | 10136 I'll apply lookup cache for not found case and see if it works.
This is exciting work! I am looking forward to more results. I feel like we definitely want to have the cache per call-site, because different GetProperty calls probably have very different objects that they are usually called on. Maybe there would be some way to re-purpose CacheIR for this as well, writing IC can be quite error prone and this way we would immediately get huge coverage.
Blocks: 906600
Severity: minor → S4
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: