Performance problem in ScriptableObject.accessSlot()

[Hannes Wallnoefer]

I think there may be a serious problem with the code in
ScriptableObject.accessSlot() that updates the slot name in order to
avoid future calls to String.equals(). I suspect that updating the
field repeatedly may have a bigger performance impact than calling
String.equals() - at least that's what my limited testing suggests. I
found that for a simple property access from the shell, the name gets
swapped not once but _twice_, and scripts that ran inexplicably slow
before now run really fast. Other scripts that have been fast before
seem unaffected. I guess it really depends on how and how often you
access properties.

Can somebody look into this?

hannes

[Hannes Wallnoefer]

Oops, please take my posting with a grain of salt. Turns out that the
_big_ performance difference was due to a change in my own code which
I had overlooked. I still think it's possilbe that the name swapping
code in ScriptableObject.accessSlot is not really an improvement, but
it probably doesn't make that big a difference.

Hannes

[Hannes Wallnoefer]

I did some real testing today and what I found is that trying to
update slot names in order to match with == rather than
String.equals() is a good idea, although the difference is really
small with modern JVMs.

The picture is really different depending on whether script code is
evaluated by the Interpreter (optimization level -1) or the native
bytecode compiler (optimization level >= 0).

With the interpreter, each string or name in the code creates a new
string instance, so different string instances are used depending on
the locality of the property name. I tried using String.intern()
during parsing/compilation, but the gained speed seems marginal and
the risk of memory clogging seems too large, especially with eval()ed
code etc.

With scripts compiled to Java bytecode, the picture is really
different from the beginning as string constants and literals in java
code are always intern()ed. So we always see the same string instance
except for strings that are composed at runtime and basically get free
== comparison hits on 99% of real world cases here, which is nice.

During my tests I also found that some standard properties (namely
error constructors and standard java package names) don't use
intern()ed property names. The reason is that they make use of a
slightly awkward string splitting mechanism during initalization.
While this is mostly a cosmetic problem I think the code could be
simplified and have filed a bug that contains a patch:

https://bugzilla.mozilla.org/show_bug.cgi?id=457705

Other than that, I think the we should keep the code in its current
form. Again, sorry for posting before doing some real testing :-)

Hannes

[Attila Szegedi]

On Sep 29, 2008, at 6:18 PM, Hannes Wallnoefer wrote:

> I did some real testing today and what I found is that trying to
> update slot names in order to match with == rather than
> String.equals() is a good idea, although the difference is really
> small with modern JVMs.
>
> The picture is really different depending on whether script code is
> evaluated by the Interpreter (optimization level -1) or the native
> bytecode compiler (optimization level >= 0).
>
> With the interpreter, each string or name in the code creates a new
> string instance

That feels wrong.

> , so different string instances are used depending on
> the locality of the property name. I tried using String.intern()
> during parsing/compilation, but the gained speed seems marginal and
> the risk of memory clogging seems too large, especially with eval()ed
> code etc.

You're aware that intern() doesn't peg a string in memory? JVM's
string interning keeps weak references to strings. If the only
reference to the string is from the interning table, it can be GCed.
So, there'd be no harm in interning.

> With scripts compiled to Java bytecode, the picture is really
> different from the beginning as string constants and literals in java
> code are always intern()ed. So we always see the same string instance
> except for strings that are composed at runtime and basically get free
> == comparison hits on 99% of real world cases here, which is nice.
>
> During my tests I also found that some standard properties (namely
> error constructors and standard java package names) don't use
> intern()ed property names. The reason is that they make use of a
> slightly awkward string splitting mechanism during initalization.
> While this is mostly a cosmetic problem I think the code could be
> simplified and have filed a bug that contains a patch:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=457705
>
> Other than that, I think the we should keep the code in its current
> form. Again, sorry for posting before doing some real testing :-)

Many language runtimes intern string literals, including fixed
identifiers found in the source code. Jython certainly does, and I
think some others do. There'd be no harm in doing this consistently
across the board.

The thing is, even if you keep String.equals() instead of ==,
String.equals() actually has a == check in it, so when the strings are
the same, you get a quick result. When they don't match, you'll need
on average l/2 comparisons to figure this out (l being average length
of a string), but the typical case is that they'll usually be
different in earlier position. So, interning identifiers and other
known string literals still gives both better CPU usage and memory
usage even if your code keeps using equals() for comparison.

Attila.

[Attila Szegedi]

Yeah, I believe last slot caching is more than likely not worth
having. Increases complexity of the code, for not much if any gain.

Attila.

On Sep 29, 2008, at 6:18 PM, Hannes Wallnoefer wrote: