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

NEW
Unassigned

Status

--
enhancement
8 years ago
9 months ago

People

(Reporter: jruderman, Unassigned)

Tracking

Trunk
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
I guess this would also turn some "is the result of the cast null?" branches into dead code.
This is just wrong.  Binary extensions can implement builtinclass interfaces.

Comment 3

8 years ago
(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.
(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.
(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.
(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.
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.
(Reporter)

Comment 8

8 years ago
(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.

Updated

9 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.