672389 - Automatically use static casts for 1-1 interface-concrete pairs

Status: NEW

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Jesse Ruderman]

Whenever an IDL interface is guaranteed to only have a single implementation, casts can be made free.  This happens when there is exactly one implementation in the tree and the interface is marked as [builtinclass] or non-scriptable.

In particular, "slow" casts such as do_QueryInterface and do_QueryObject can safely be replaced by the "free" static_cast<>.

With a static analysis script, we could automatically identify safe interface/class pairs and...

A) Replace slow casts with static_cast<>.

B) Replace slow casts with a macro that does static_cast<> while signaling to the analysis "I statically assert that this is safe; tell me if it becomes unsafe".

C) Allow compilers to automatically apply the optimization without source changes.

This will provide (tiny?) performance wins throughout our code and reduce the temptation to apply the optimization unsafely by hand.

Comment 1

[Jesse Ruderman]

I guess this would also turn some "is the result of the cast null?" branches into dead code.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is just wrong.  Binary extensions can implement builtinclass interfaces.

Comment 3

[(dormant account)]

(In reply to comment #2)
> This is just wrong.  Binary extensions can implement builtinclass interfaces.

they can..and they will cause crashes. All of the stuff in Services.h should be safe for this.

Comment 4

[Joshua Cranmer [:jcranmer]]

(In reply to comment #2)
> This is just wrong.  Binary extensions can implement builtinclass interfaces.

Perhaps we ought to change the semantics of [builtinclass] (or add a similar attribute) to "we have one canonical implementation and THAT'S IT"--i.e., preventing overriding in component registrar.

But I totally agree that just because an interface isn't scriptable, we shouldn't assume that this is the case.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #3)
> (In reply to comment #2)
> > This is just wrong.  Binary extensions can implement builtinclass interfaces.
> 
> they can..and they will cause crashes. All of the stuff in Services.h should
> be safe for this.

Sure, some things are safe, but some aren't.  If we're going to assert that builtinclass interfaces must have no non-libxul implementations we should make that explicit before we go running around marking things as builtinclass.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #4)
> (In reply to comment #2)
> > This is just wrong.  Binary extensions can implement builtinclass interfaces.
> 
> Perhaps we ought to change the semantics of [builtinclass] (or add a similar
> attribute) to "we have one canonical implementation and THAT'S IT"--i.e.,
> preventing overriding in component registrar.

Sure, but that applies to components, not interfaces.  And if we wanted to stick a bit in the xpcom registration goop that says "don't allow overriding this contract id" I'd ask why we don't just construct by CID to begin with.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Binary addons can always do anything that'll crash us. All we can do is to say "Don't implement interfaces marked with [builtinclass]".

For what it's worth, i think this bug is WONTFIX. At the most we could have a tool that marks potential optimizations, but replacing them all automatically runs a terribly large risk that we'll add additional implementations in the future.

Comment 8

[Jesse Ruderman]

(B) and (C) would ensure the optimization goes away if you add additional implementations in the future.

A fourth option is:

D) Only do this optimizations for interfaces that explicitly claim to have only one implementation, e.g. [builtinclass(nsConcreteClass)].

Then super-reviewers would make sure that if you remove the annotation, you also scour the codebase for unsafe casts and other no-longer-valid assumptions.