GNOME Bugzilla – Bug 697592
[performance] Reduce time spent in property access
Last modified: 2013-04-23 20:56:58 UTC
Recently I got around to do some profiling in gjs, and got amazed by the amount of time spent doing utf-8 conversion just to intern property names used internally. This caused gjs_keep_alive_get_global() to be over 7% (in callgrind instruction fetches). The following patch stack reduces it to 0.22%.
Created attachment 240986 [details] [review] Add a way to store commonly used property names Accessing JS properties by name is expensive, because it forces the JS API to convert them to UTF-8 and atomize them. We can avoid that by interning property names that we know we need, and keeping the jsid at hand.
Created attachment 240987 [details] [review] KeepAlive: use an interned jsid for keep alive name The keep alive object is accessed very often, so it definitely benefits from using an already interned jsid instead of converting and hashing the same string again and again.
Created attachment 240988 [details] [review] Use interned jsid for "prototype" Replace all accesses to the prototype property with the jsid variant. This involves one path, which is object construction.
Created attachment 240990 [details] [review] Replace remaining get_property with interned jsids None of these are hot paths, but it helps anyway.
Created attachment 240991 [details] [review] Get rid of gjs_object_get_property and gjs_object_has_property They needlessly save and restore the exception state, which is expensive. Callers that need it should implement it themselves, but they don't really, so just have them call JS_GetProperty directly.
Created attachment 240992 [details] [review] Make gjs_object_require_property deal with ids instead of names This way we can cache the IDs.
Created attachment 240993 [details] [review] Don't use a property for the keep alive The keep alive is accessed so often that even using property IDs is slow. Introduce the concept of "global slots" instead, which are special hidden properties hanging off the global object. We can't use regular reserved slots, because the global objects uses some internally, so we emulate them with the private data and a custom tracer. While we're there, let's do the same for imports.
Review of attachment 240986 [details] [review]: Interesting. Yes, this looks reasonable.
Review of attachment 240987 [details] [review]: Looks good.
Review of attachment 240988 [details] [review]: One comment: ::: gi/boxed.c @@ +1201,3 @@ + prototype_name = gjs_runtime_get_const_string(JS_GetRuntime(context), + GJS_STRING_PROTOTYPE); + JS_GetPropertyById(context, constructor, prototype_name, &value); There are so many calls like this, I wonder if it'd be worth having a: gjs_get_property_const(context, constructor, GJS_STRING_PROTOTYPE, &value))
Review of attachment 240990 [details] [review]: Looks fine.
Review of attachment 240991 [details] [review]: This looks OK to me; but do you feel confident there's no places in the code which were relying on the exception state trapping?
Review of attachment 240992 [details] [review]: This looks correct to me. We're certainly interning far more strings than we used to, but that's fine because they're one per type, not per instance or anything like that.
Review of attachment 240993 [details] [review]: This looks reasonable, but can you explain: what's your performance measuring methodology here?
(In reply to comment #12) > Review of attachment 240991 [details] [review]: > > This looks OK to me; but do you feel confident there's no places in the code > which were relying on the exception state trapping? I don't think there are places that explicitly rely on it. It's possible we introduced new failure path if a property is an accessor and that fails, but then again, eating the exception silently is probably not the right thing. (In reply to comment #14) > Review of attachment 240993 [details] [review]: > > This looks reasonable, but can you explain: what's your performance measuring > methodology here? I callgrinded #!/usr/bin/env gjs const Everything = imports.gi.Regress; for(var i=0; i < 100000; i++) Everything.test_torture_signature_2(42, function () {}, 'foo', 7); Not very reliable, but it should optimize away property accesses from the language, and thus test the marshalling part, which was my primary objective when I started this.
Created attachment 241919 [details] [review] Add a convenience API for const property access Rather than copy pasting everywhere gjs_runtime_get_const_string() followed by JS_GetPropertyById, add a function that wraps it using an declaration similar to JS_GetProperty().
Review of attachment 241919 [details] [review]: Looks good!
Attachment 240986 [details] pushed as 4d378a6 - Add a way to store commonly used property names Attachment 240987 [details] pushed as 915ac72 - KeepAlive: use an interned jsid for keep alive name Attachment 240988 [details] pushed as f3ccf49 - Use interned jsid for "prototype" Attachment 240990 [details] pushed as 9cea91b - Replace remaining get_property with interned jsids Attachment 240991 [details] pushed as 4601f36 - Get rid of gjs_object_get_property and gjs_object_has_property Attachment 240992 [details] pushed as dfa7ce5 - Make gjs_object_require_property deal with ids instead of names Attachment 240993 [details] pushed as 4260621 - Don't use a property for the keep alive
Attachment 241919 [details] pushed as d692f8e - Add a convenience API for const property access