Proposal to stop revving UUIDs when changing XPIDL interfaces

[Ehsan Akhgari]

Historically we have enforced updating the XPIDL interface UUIDs when
you made any changes to it. This was needed because of two reasons:

* Backwards compatibility with binary extensions. Since many changes to
XPIDL interfaces caused the underlying v-table layout to change, revving
the UUID enabled previously compiled extensions to fail getting the
interface through QueryInterface() in the first place, preventing
crashes when they try to use the interface.

* Incremental builds. Our build system used to not repack the compiled
XPT file unless it detected a change in the UUID, which would manifest
as weird issues when you landed code changing an interface without
changing its UUID, in that in incremental builds the XPT file would be
outdated, but in clobbered builds it would be correct.

We have created Mercurial hooks that enforce a UUID change when an idl
file is touched because of these requirements.

Ever since Firefox 41, we have stopped supporting binary components in
extensions, so the first reason doesn’t apply any more. And since
yesterday I have fixed bug 977464 which fixes the second issue. So as
far as I can tell, there is no reason to keep revving UUIDs any more.
Therefore I would like to propose that we should remove the Mercurial
hook (bug 1170718) and relax this requirement on trunk, and let this
ride the trains.

Three points worth mentioning here.

* Thunderbird still supports binary components in extensions. In
<https://bugzilla.mozilla.org/show_bug.cgi?id=977464#c31> Kent said that
Thunderbird is OK with change.

* My proposal has no bearing on whether changes to XPIDL interfaces
needs to be considered as part of the uplift approval process, as such
changes can still have an impact on JS extension compatibility.
Therefore under my proposal we’d reword the approval canned
questionnaire on Bugzilla to talk about changes to XPIDL interfaces in
addition to string changes, in lieu of mentioning UUID changes.

* UUIDs are still the unique identifiers used in QueryInterface()
implementations and you'd still need to tag the interface with a UUID
when you create a new XPIDL interface.

Please let me know if you have any questions or concerns.

Cheers,
Ehsan

[Kyle Huey]

As the XPIDL module owner, I support this.

- Kyle

On Fri, Jan 15, 2016 at 7:58 AM, Ehsan Akhgari <ehsan....@gmail.com>
wrote:

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

[Boris Zbarsky]

On 1/15/16 10:58 AM, Ehsan Akhgari wrote:
> * My proposal has no bearing on whether changes to XPIDL interfaces
> needs to be considered as part of the uplift approval process, as such
> changes can still have an impact on JS extension compatibility.

This should probably include Web IDL interfaces too, then, right?

-Boris

[Patrick McManus]

On Fri, Jan 15, 2016 at 10:58 AM, Ehsan Akhgari <ehsan....@gmail.com>
wrote:

> Please let me know if you have any questions or concerns.

or cheers.

cheers!

[Bobby Holley]

Has anyone measured recently whether there's still a significant perf win
to making IIDs 32-bit? If we stop using them as a versioning tool, we could
potentially relax our uniqueness requirements, and save a lot of
comparisons on each QI. Addon-compat would be tricky, but is potentially
solvable.

It may be that we've optimized enough stuff that we don't QI on the hot
paths anymore. But it might be worth a day of someone's time to check if it
makes any difference on Talos.

On Fri, Jan 15, 2016 at 10:55 AM, Patrick McManus <mcm...@ducksong.com>
wrote:

[Joshua Cranmer 🐧]

On 1/15/2016 1:21 PM, Bobby Holley wrote:
> Has anyone measured recently whether there's still a significant perf win
> to making IIDs 32-bit? If we stop using them as a versioning tool, we could
> potentially relax our uniqueness requirements, and save a lot of
> comparisons on each QI. Addon-compat would be tricky, but is potentially
> solvable.

Are we still using nsISupports in a way that we expect it to be
ABI-compatible with IUnknown?

--
Joshua Cranmer
Thunderbird and DXR developer
Source code archæologist

[Ehsan Akhgari]

On 2016-01-15 2:21 PM, Bobby Holley wrote:
> Has anyone measured recently whether there's still a significant perf
> win to making IIDs 32-bit? If we stop using them as a versioning tool,
> we could potentially relax our uniqueness requirements, and save a lot
> of comparisons on each QI. Addon-compat would be tricky, but is
> potentially solvable.
>

> It may be that we've optimized enough stuff that we don't QI on the hot
> paths anymore. But it might be worth a day of someone's time to check if
> it makes any difference on Talos.

Years ago I wasted a tremendous amount of time working on bug 391275 to
remove the QueryInterface() outparam argument and couldn't measure any
performance difference whatsoever, which makes me be extremely cautious
when assuming that making QueryInterface() faster by shortening the
UUIDs would move any needles. (But I could be wrong, of course!)

[Ehsan Akhgari]

Yes.

[Jonas Sicking]

On Fri, Jan 15, 2016 at 11:41 AM, Joshua Cranmer 🐧 <Pidg...@gmail.com> wrote:
> On 1/15/2016 1:21 PM, Bobby Holley wrote:
>>
>> Has anyone measured recently whether there's still a significant perf win
>> to making IIDs 32-bit? If we stop using them as a versioning tool, we
>> could
>> potentially relax our uniqueness requirements, and save a lot of
>> comparisons on each QI. Addon-compat would be tricky, but is potentially
>> solvable.
>
>
> Are we still using nsISupports in a way that we expect it to be
> ABI-compatible with IUnknown?

Last I checked (which was years ago), the accessibility code still
did. More specifically it implement MSCOM and XPCOM interfaces on the
same object.

Which is why we're forced to use stdcall and thus why we're using the
NS_IMETHOD* macros.

See also https://bugzilla.mozilla.org/show_bug.cgi?id=662348

Trevor, do you know if this has been fixed in the accessibility code since?

/ Jonas

[Trevor Saunders]

On Fri, Jan 15, 2016 at 04:28:13PM -0800, Jonas Sicking wrote:
> On Fri, Jan 15, 2016 at 11:41 AM, Joshua Cranmer 🐧 <Pidg...@gmail.com> wrote:

> > On 1/15/2016 1:21 PM, Bobby Holley wrote:
> >>
> >> Has anyone measured recently whether there's still a significant perf win
> >> to making IIDs 32-bit? If we stop using them as a versioning tool, we
> >> could
> >> potentially relax our uniqueness requirements, and save a lot of
> >> comparisons on each QI. Addon-compat would be tricky, but is potentially
> >> solvable.
> >
> >
> > Are we still using nsISupports in a way that we expect it to be
> > ABI-compatible with IUnknown?
>

> Last I checked (which was years ago), the accessibility code still
> did. More specifically it implement MSCOM and XPCOM interfaces on the
> same object.
>
> Which is why we're forced to use stdcall and thus why we're using the
> NS_IMETHOD* macros.
>
> See also https://bugzilla.mozilla.org/show_bug.cgi?id=662348

The accessibility code still does that. we've moved away from it some,
but completely fixing it has never been really high priority. I've never
been totally clear on how that over loading manages to work, but I
wonder if changing to nsresult QueryInterface(int, void**) would keep
the two methods separate enough that it would be ok.

Trev

>
> Trevor, do you know if this has been fixed in the accessibility code since?
>
> / Jonas

[Ehsan Akhgari]

On 2016-01-15 7:44 PM, Trevor Saunders wrote:
> On Fri, Jan 15, 2016 at 04:28:13PM -0800, Jonas Sicking wrote:
>> On Fri, Jan 15, 2016 at 11:41 AM, Joshua Cranmer 🐧 <Pidg...@gmail.com> wrote:

>>> On 1/15/2016 1:21 PM, Bobby Holley wrote:
>>>>
>>>> Has anyone measured recently whether there's still a significant perf win
>>>> to making IIDs 32-bit? If we stop using them as a versioning tool, we
>>>> could
>>>> potentially relax our uniqueness requirements, and save a lot of
>>>> comparisons on each QI. Addon-compat would be tricky, but is potentially
>>>> solvable.
>>>
>>>
>>> Are we still using nsISupports in a way that we expect it to be
>>> ABI-compatible with IUnknown?
>>

>> Last I checked (which was years ago), the accessibility code still
>> did. More specifically it implement MSCOM and XPCOM interfaces on the
>> same object.
>>
>> Which is why we're forced to use stdcall and thus why we're using the
>> NS_IMETHOD* macros.
>>
>> See also https://bugzilla.mozilla.org/show_bug.cgi?id=662348
>
> The accessibility code still does that. we've moved away from it some,
> but completely fixing it has never been really high priority. I've never
> been totally clear on how that over loading manages to work, but I
> wonder if changing to nsresult QueryInterface(int, void**) would keep
> the two methods separate enough that it would be ok.

Hmm, it looks like there are already separate overloads now... See:

<https://dxr.mozilla.org/mozilla-central/source/accessible/windows/msaa/AccessibleWrap.cpp#88>
<https://dxr.mozilla.org/mozilla-central/source/accessible/windows/msaa/AccessibleWrap.cpp#110>

So maybe this is no longer an issue!

[Honza Bambas]

On 1/15/2016 21:02, Ehsan Akhgari wrote:

> On 2016-01-15 2:21 PM, Bobby Holley wrote:
>> Has anyone measured recently whether there's still a significant perf
>> win to making IIDs 32-bit? If we stop using them as a versioning tool,
>> we could potentially relax our uniqueness requirements, and save a lot
>> of comparisons on each QI. Addon-compat would be tricky, but is
>> potentially solvable.
>>

>> It may be that we've optimized enough stuff that we don't QI on the hot
>> paths anymore. But it might be worth a day of someone's time to check if
>> it makes any difference on Talos.
>
> Years ago I wasted a tremendous amount of time working on bug 391275
> to remove the QueryInterface() outparam argument and couldn't measure
> any performance difference whatsoever, which makes me be extremely
> cautious when assuming that making QueryInterface() faster by
> shortening the UUIDs would move any needles. (But I could be wrong,
> of course!)

Just a note that whenever I profile gecko with an external profiler
(like CodeAnalyst) I can see we spend enormous amount of time in
QueryInterface. It's more about number of calls than what time we
actually spend in it, but optimizing it may have some influence. Same as
an attempt to order interfaces in NS_ISUPPORTS_IMPL in order according
how often we perform QI to each interface.

-hb-

[smaug]

On 01/18/2016 06:03 PM, Honza Bambas wrote:
> On 1/15/2016 21:02, Ehsan Akhgari wrote:
>> On 2016-01-15 2:21 PM, Bobby Holley wrote:
>>> Has anyone measured recently whether there's still a significant perf
>>> win to making IIDs 32-bit? If we stop using them as a versioning tool,
>>> we could potentially relax our uniqueness requirements, and save a lot
>>> of comparisons on each QI. Addon-compat would be tricky, but is
>>> potentially solvable.
>>>
>>> It may be that we've optimized enough stuff that we don't QI on the hot
>>> paths anymore. But it might be worth a day of someone's time to check if
>>> it makes any difference on Talos.
>>
>> Years ago I wasted a tremendous amount of time working on bug 391275 to remove the QueryInterface() outparam argument and couldn't measure any
>> performance difference whatsoever, which makes me be extremely cautious when assuming that making QueryInterface() faster by shortening the UUIDs
>> would move any needles. (But I could be wrong, of course!)
>
> Just a note that whenever I profile gecko with an external profiler (like CodeAnalyst) I can see we spend enormous amount of time in QueryInterface.
> It's more about number of calls than what time we actually spend in it, but optimizing it may have some influence. Same as an attempt to order
> interfaces in NS_ISUPPORTS_IMPL in order according how often we perform QI to each interface.

It is indeed usually the cost of virtual call, and virtual AddRef call inside QueryInterface. IID handling less so.

-Olli

[Ehsan Akhgari]

10 days and no objections. This is now the new rule! Please stop updating
UUIDs when changing XPIDL interfaces.

On Fri, Jan 15, 2016 at 10:58 AM, Ehsan Akhgari <ehsan....@gmail.com>

> * My proposal has no bearing on whether changes to XPIDL interfaces needs
> to be considered as part of the uplift approval process, as such changes

> can still have an impact on JS extension compatibility. Therefore under my
> proposal we’d reword the approval canned questionnaire on Bugzilla to talk
> about changes to XPIDL interfaces in addition to string changes, in lieu of
> mentioning UUID changes.
>
> * UUIDs are still the unique identifiers used in QueryInterface()
> implementations and you'd still need to tag the interface with a UUID when
> you create a new XPIDL interface.
>

> Please let me know if you have any questions or concerns.
>

> Cheers,
> Ehsan
>



--
Ehsan

[Eric Rahm]

Have the reject-on-idl-change-but-no-uuid-change scripts been updated on the hg server?

[Ehsan Akhgari]

On 2016-01-28 7:24 PM, Eric Rahm wrote:
> Have the reject-on-idl-change-but-no-uuid-change scripts been updated on the hg server?

Yes (on m-c and branches that merge to it.)