Andrew Drake [:adrake] Reporter
	Description • 14 years ago

Need to fix the (minor) performance hit caused by the quick fix to bug https://bugzilla.mozilla.org/show_bug.cgi?id=577646 .

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 1 • 14 years ago

Suggestion: Don't keep track of JSClass, let's introduce an ObjectType enum somewhere.

Looking forward to this patch. With it we can later keep track of the compiler-created JSFunction for closure-creating opcodes. In these cases we can make calls very fast.

	Andrew Drake [:adrake] Reporter
	Comment 2 • 14 years ago

So I threw together this quick implementation. Somehow, it slows down SunSpider by about 2%. I can't seem to get any consistent numbers for v8, but it looks like it generally slows down there, too. Am I doing something stupid? It seems like a strict reduction in the amount of compiled code, and I can't believe it's introducing 8 extra ms of compile overhead.

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 3 • 14 years ago

(In reply to comment #1)
> Suggestion: Don't keep track of JSClass, let's introduce an ObjectType enum
> somewhere.

jsproto.tbl probably has what you need.

	Brendan Eich [:brendan]
	Comment 4 • 14 years ago

For the builtin classes, JSCLASS_CACHED_PROTO_KEY(clasp) goes from JSClass *clasp to JSProtoKey.

/be

	Sean Stangl [:sstangl]
	Comment 5 • 14 years ago

It was requested that I time benchmarks with the patch from #2 on my local computer, since the older processor occasionally gives different results.

With the above patch, SunSpider does not change performance, but v8 gets slightly faster. Only v8/earley-boyer is affected, receiving a 2% speedup, since calls to a lambda now save a few instructions. The v8 testsuite as a whole is 0.5% faster, consistently.

	Sean Stangl [:sstangl]
	Comment 6 • 14 years ago

From the patch in #2:

> +    if (typeKnown && 
> +        (fe->getKnownType() != JSVAL_TYPE_OBJECT ||
> +         (fe->isObjectTypeKnown() && fe->getKnownObjectType() != JSOBJECT_FUNCTION))) {

and later..

> +    if (!typeKnown || !fe->isObjectTypeKnown()) {
> +        j = masm.testFunction(Assembler::NotEqual, data);
> +        stubcc.linkExit(j, Uses(argc + 2));
> +        stubcc.leave();
> +        stubcc.masm.move(Imm32(argc), Registers::ArgReg1);
> +        stubcc.call(callingNew ? stubs::SlowNew : stubs::SlowCall);
> +    }

This code assumes that fe->isObjectTypeKnown() implies typeKnown. This is not the case: it occurs that !typeKnown && fe->isObjectTypeKnown() && fe->getKnownObjectType != JSOBJECT_FUNCTION. Perhaps frame.forgetEverything()?

	Andrew Drake [:adrake] Reporter
	Comment 7 • 14 years ago

We hold the invariant that isObjectTypeKnown() is only called if isTypeKnown and knownType is JSVAL_TYPE_OBJECT, so any result returned from such a situation in a release build is bogus. This trips an assertion in a debug build.

	Sean Stangl [:sstangl]
	Comment 8 • 14 years ago

Above

> if(!typeKnown || !fe->isObjectTypeKnown()) {

I inserted the following logging code:

> printf("%d %d %d\n", typeKnown, typeKnown && fe->isObjectTypeKnown(), typeKnown && fe->isObjectTypeKnown() && fe->getKnownObjectType() == JSOBJECT_FUNCTION);

Running this on the v8 suite yields:

 0 0 0 - 3965 times
 1 1 1 -    1 time

Running this on the SunSpider suite yields:

 0 0 0 - 674 times
 1 1 1 -   6 times

	Andrew Drake [:adrake] Reporter
	Comment 9 • 14 years ago

The above patch fails several trace-tests and appears to cause stack corruption. Need to investigate this tomorrow.

	Andrew Drake [:adrake] Reporter
	Comment 10 • 14 years ago

This turns out to be pretty complicated, and eliminates a grand total of 7 guards without significantly more sophistication. It'd still be nice to do as far as tiny wins go, but it may be subsumed by type inference.