Conversion from PR_TRUE and PR_FALSE to true and false

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

Conversion from PR_TRUE and PR_FALSE to true and false

Ehsan Akhgari
I took it upon myself to finish the other half of Michael Wu's awesome work
this week: which is switching from PR_TRUE/PR_FALSE to true/false on
mozilla-central.  I've filed bug 690892 for this.

If everything goes as planned, I'm planning to close mozilla-central and
mozilla-inbound on Monday, Oct 3 at 8:00AM PST, do a merge, and then land
the patch.  The trees will remain closed while we get green runs.

I've written a script which people can use to unbitrot their patch queues
once this lands.  You can find it at
https://bugzilla.mozilla.org/attachment.cgi?id=563846.

I will update here once the change has been landed successfully and the
trees have been reopened.

Cheers,
--
Ehsan
<http://ehsanakhgari.org/>
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Conversion from PR_TRUE and PR_FALSE to true and false

Boris Zbarsky
On 9/30/11 5:26 PM, Ehsan Akhgari wrote:
> I took it upon myself to finish the other half of Michael Wu's awesome work
> this week: which is switching from PR_TRUE/PR_FALSE to true/false on
> mozilla-central.  I've filed bug 690892 for this.
>
> If everything goes as planned, I'm planning to close mozilla-central and
> mozilla-inbound on Monday, Oct 3 at 8:00AM PST, do a merge, and then land
> the patch.  The trees will remain closed while we get green runs.

I would vote for sorting our the regressions from the patch that already
landed before landing more things on top of it...

-Boris
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Conversion from PR_TRUE and PR_FALSE to true and false

Mike Shaver
On Fri, Sep 30, 2011 at 10:50 PM, Boris Zbarsky <[hidden email]> wrote:
> I would vote for sorting our the regressions from the patch that already
> landed before landing more things on top of it...

I think this might actually help with the performance regressions,
since it will eliminate a bunch of int->bool conversions.

Mike
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Conversion from PR_TRUE and PR_FALSE to true and false

Boris Zbarsky
In reply to this post by Boris Zbarsky
On 10/1/11 7:27 PM, Mike Shaver wrote:
> On Fri, Sep 30, 2011 at 10:50 PM, Boris Zbarsky<[hidden email]>  wrote:
>> I would vote for sorting our the regressions from the patch that already
>> landed before landing more things on top of it...
>
> I think this might actually help with the performance regressions,
> since it will eliminate a bunch of int->bool conversions.

OK, fair.  I suppose we can give it a shot...

Note that there are also correctness regressions, by the way.

-Boris

_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Conversion from PR_TRUE and PR_FALSE to true and false

Joshua Cranmer-2
In reply to this post by Boris Zbarsky
On 10/1/2011 6:27 PM, Mike Shaver wrote:
> On Fri, Sep 30, 2011 at 10:50 PM, Boris Zbarsky<[hidden email]>  wrote:
>> I would vote for sorting our the regressions from the patch that already
>> landed before landing more things on top of it...
> I think this might actually help with the performance regressions,
> since it will eliminate a bunch of int->bool conversions.
>
Since PR_TRUE and PR_FALSE are compile-time constants, any optimizer
should easily be able to eliminate the int->bool conversions caused by
these not actually being true and false, respectively...
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Conversion from PR_TRUE and PR_FALSE to true and false

Bobby Holley-2
If we don't want to do another tree-wide patch right now, why not just
#define PR_TRUE and PR_FALSE to true and false (#ifdef __cplusplus, of
course). That should eliminate any potential performance regressions, real
or imagined.

-bholley

On Sun, Oct 2, 2011 at 3:41 PM, Joshua Cranmer <[hidden email]>wrote:

> On 10/1/2011 6:27 PM, Mike Shaver wrote:
>
>> On Fri, Sep 30, 2011 at 10:50 PM, Boris Zbarsky<[hidden email]>  wrote:
>>
>>> I would vote for sorting our the regressions from the patch that already
>>>
>>> landed before landing more things on top of it...
>>>
>> I think this might actually help with the performance regressions,
>>
>> since it will eliminate a bunch of int->bool conversions.
>>
>>  Since PR_TRUE and PR_FALSE are compile-time constants, any optimizer
> should easily be able to eliminate the int->bool conversions caused by these
> not actually being true and false, respectively...
>
> ______________________________**_________________
> dev-planning mailing list
> [hidden email]
> https://lists.mozilla.org/**listinfo/dev-planning<https://lists.mozilla.org/listinfo/dev-planning>
>
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Conversion from PR_TRUE and PR_FALSE to true and false

L. David Baron
On Sunday 2011-10-02 23:52 -0400, Bobby Holley wrote:
> If we don't want to do another tree-wide patch right now, why not just
> #define PR_TRUE and PR_FALSE to true and false (#ifdef __cplusplus, of
> course). That should eliminate any potential performance regressions, real
> or imagined.

The reason to do another tree-wide patch is that module owners
mostly appear to want to move to the standard C++ syntax, but having
each module do the substitution at a different time makes it much
harder for people with patch queues to unbitrot their patches.
(One or two have already landed, and a few more were coming down the
pipe.)

If it's a tree-wide change, then everybody can just search-replace
their entire patch queues (unless they have NSPR or NSS patches).

-David

--
𝄞   L. David Baron                         http://dbaron.org/   𝄂
𝄢   Mozilla                           http://www.mozilla.org/   𝄂
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Conversion from PR_TRUE and PR_FALSE to true and false

Bobby Holley-2
Sure. I was just offering this as a middle ground between bz's desire to
wait a little bit before the next tree-wide patch and shaver's desire to
eliminate potential performance regressions. I'm equally open to the idea
that we shouldn't wait.

-bholley

On Sun, Oct 2, 2011 at 11:57 PM, L. David Baron <[hidden email]> wrote:

> On Sunday 2011-10-02 23:52 -0400, Bobby Holley wrote:
> > If we don't want to do another tree-wide patch right now, why not just
> > #define PR_TRUE and PR_FALSE to true and false (#ifdef __cplusplus, of
> > course). That should eliminate any potential performance regressions,
> real
> > or imagined.
>
> The reason to do another tree-wide patch is that module owners
> mostly appear to want to move to the standard C++ syntax, but having
> each module do the substitution at a different time makes it much
> harder for people with patch queues to unbitrot their patches.
> (One or two have already landed, and a few more were coming down the
> pipe.)
>
> If it's a tree-wide change, then everybody can just search-replace
> their entire patch queues (unless they have NSPR or NSS patches).
>
> -David
>
> --
> 𝄞   L. David Baron                         http://dbaron.org/   𝄂
> 𝄢   Mozilla                           http://www.mozilla.org/   𝄂
>
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Conversion from PR_TRUE and PR_FALSE to true and false

Boris Zbarsky
In reply to this post by L. David Baron
On 10/3/11 12:02 AM, Bobby Holley wrote:
> Sure. I was just offering this as a middle ground between bz's desire to
> wait a little bit before the next tree-wide patch

My desire is not to wait before the next tree-wide patch but to have a
plan for what happens if we don't find a fix for the regressions.

In the tree-wide patch approach the plan seems to be to back out the
original switch and everything that landed after the treewide thing
(since it'll be near-impossible to merge a backout of just the original
switch) or to just accept the regressions, right?

In the "wait a week" approach the plan is to back out the original
switch (or of course to accept the regressions).

-Boris
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Conversion from PR_TRUE and PR_FALSE to true and false

Boris Zbarsky
On 10/3/11 12:53 AM, Boris Zbarsky wrote:
> On 10/3/11 12:02 AM, Bobby Holley wrote:
>> Sure. I was just offering this as a middle ground between bz's desire to
>> wait a little bit before the next tree-wide patch
>
> My desire is not to wait before the next tree-wide patch but to have a
> plan for what happens if we don't find a fix for the regressions.

And in particular I am just as opposed (which is somewhat, but not to
the point of really fighting over it) to piecemeal changes to change
PR_TRUE/FALSE to true/false as I am to a treewide change.  Certainly a
treewide change is preferable to piecemeal ones for the reasons dbaron
mentioned; I happen to think we should wait a bit before doing either of
them.

-Boris
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Conversion from PR_TRUE and PR_FALSE to true and false

Ehsan Akhgari
I decided that I will hold off on landing this change for now, based on
Boris' objection.  But I would like to wait for a known amount of time (say
a week).  We should have a plan on what to do if we can't figure out the
regression by then.

Also, FWIW I compared the Linux Dromaeo CSS numbers after my path on the try
server, and the regression doesn't seem to be affected by my patch.

Cheers,
--
Ehsan
<http://ehsanakhgari.org/>


On Mon, Oct 3, 2011 at 12:55 AM, Boris Zbarsky <[hidden email]> wrote:

> On 10/3/11 12:53 AM, Boris Zbarsky wrote:
>
>> On 10/3/11 12:02 AM, Bobby Holley wrote:
>>
>>> Sure. I was just offering this as a middle ground between bz's desire to
>>> wait a little bit before the next tree-wide patch
>>>
>>
>> My desire is not to wait before the next tree-wide patch but to have a
>> plan for what happens if we don't find a fix for the regressions.
>>
>
> And in particular I am just as opposed (which is somewhat, but not to the
> point of really fighting over it) to piecemeal changes to change
> PR_TRUE/FALSE to true/false as I am to a treewide change.  Certainly a
> treewide change is preferable to piecemeal ones for the reasons dbaron
> mentioned; I happen to think we should wait a bit before doing either of
> them.
>
>
> -Boris
> ______________________________**_________________
> dev-planning mailing list
> [hidden email]
> https://lists.mozilla.org/**listinfo/dev-planning<https://lists.mozilla.org/listinfo/dev-planning>
>
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Conversion from PR_TRUE and PR_FALSE to true and false

Boris Zbarsky
In reply to this post by Boris Zbarsky
On 10/3/11 9:32 AM, Ehsan Akhgari wrote:
> I decided that I will hold off on landing this change for now, based on
> Boris' objection.  But I would like to wait for a known amount of time (say
> a week).

I think a week should be plenty of time for us to get a good handle on
the regression situation.  If you're willing to wait a week, that sounds
great.

Thanks!

-Boris
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Conversion from PR_TRUE and PR_FALSE to true and false

Marco Bonardo-2
In reply to this post by Boris Zbarsky
On 03/10/2011 15:32, Ehsan Akhgari wrote:
> I decided that I will hold off on landing this change for now, based on
> Boris' objection.  But I would like to wait for a known amount of time (say
> a week).  We should have a plan on what to do if we can't figure out the
> regression by then.

Actually, there's a problem with merges.
Since someone started doing partial conversions in some components, when
merging central in inbound there is an high opportunity for merge
conflicts. Today I had to do a manual merge due to this, Ms2ger touched
code that was converted to true/false in inbound and this caused some
headache.
If we don't do the full tree conversion now, we should ask people to not
do single components conversions as well (these are already on top of
the PRBool change so I can't see how we'd proceed in case of backout).
Can't we do the full conversion and when the regression is found, if
there is no other fix possible, reconvert only that code to PRBool?

Marco

_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Conversion from PR_TRUE and PR_FALSE to true and false

Boris Zbarsky
On 10/3/11 9:52 AM, Marco Bonardo wrote:
> If we don't do the full tree conversion now, we should ask people to not
> do single components conversions as well

Yes, absolutely.  People should NOT be doing those, as dbaron said.

I thought not landing things depending on a big landing until the
landing has stuck well (in our rapid release world possibly until the
_next_ cycle so the landing can be backed out on aurora if needed!) was
basic sanity.  Apparently I was wrong.  :(

> Can't we do the full conversion and when the regression is found, if
> there is no other fix possible, reconvert only that code to PRBool?

You're assuming that reconverting a particular bit of code to PRBool
will fix the regressions.

I'm worried about regressions that we simply don't get a handle on.
Again, in a few days we should have way more information here.

-Boris
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Conversion from PR_TRUE and PR_FALSE to true and false

Ehsan Akhgari
In reply to this post by Marco Bonardo-2
On Mon, Oct 3, 2011 at 9:52 AM, Marco Bonardo <[hidden email]> wrote:

> On 03/10/2011 15:32, Ehsan Akhgari wrote:
>
>> I decided that I will hold off on landing this change for now, based on
>> Boris' objection.  But I would like to wait for a known amount of time
>> (say
>> a week).  We should have a plan on what to do if we can't figure out the
>> regression by then.
>>
>
> Actually, there's a problem with merges.
> Since someone started doing partial conversions in some components, when
> merging central in inbound there is an high opportunity for merge conflicts.
> Today I had to do a manual merge due to this, Ms2ger touched code that was
> converted to true/false in inbound and this caused some headache.
> If we don't do the full tree conversion now, we should ask people to not do
> single components conversions as well (these are already on top of the
> PRBool change so I can't see how we'd proceed in case of backout).
> Can't we do the full conversion and when the regression is found, if there
> is no other fix possible, reconvert only that code to PRBool?


FWIW, my rule of thumb is this.  If somebody lands a large patch on
mozilla-inbound touching many files which is conflicting with everything
landed in mozilla-central, I would back out that patch from
mozilla-inbound.  The mozilla-inbound sheriffs cannot be expected to resolve
merge conflicts beyond extremely simple ones, so, as always, when in doubt,
back out.  ;-)  The correct way for landing large changes which have the
potential of conflicting with many other patches is to land on either of
mozilla-central or mozilla-inbound and then immediately merge with the other
branch.

--
Ehsan
<http://ehsanakhgari.org/>
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Conversion from PR_TRUE and PR_FALSE to true and false

Ehsan Akhgari
In reply to this post by Ehsan Akhgari
Now that bug 690864 is WONTFIXed, this is on schedule again to happen on
Monday, Oct 17 at 8:00AM PST.  mozilla-central and mozilla-inbound will be
closed while we get green runs from this.  Please let me know if you have
any concerns about this before then.

Thanks!
--
Ehsan
<http://ehsanakhgari.org/>


On Fri, Sep 30, 2011 at 5:26 PM, Ehsan Akhgari <[hidden email]>wrote:

> I took it upon myself to finish the other half of Michael Wu's awesome work
> this week: which is switching from PR_TRUE/PR_FALSE to true/false on
> mozilla-central.  I've filed bug 690892 for this.
>
> If everything goes as planned, I'm planning to close mozilla-central and
> mozilla-inbound on Monday, Oct 3 at 8:00AM PST, do a merge, and then land
> the patch.  The trees will remain closed while we get green runs.
>
> I've written a script which people can use to unbitrot their patch queues
> once this lands.  You can find it at
> https://bugzilla.mozilla.org/attachment.cgi?id=563846.
>
> I will update here once the change has been landed successfully and the
> trees have been reopened.
>
> Cheers,
> --
> Ehsan
> <http://ehsanakhgari.org/>
>
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Conversion from PR_TRUE and PR_FALSE to true and false

Ehsan Akhgari
This work is now complete.  Except for a few directories, there should not
be any more usages of PR_TRUE and PR_FALSE, and I would appreciate if people
didn't add more of those in their new patches.  :-)

You can use the script in
https://bugzilla.mozilla.org/attachment.cgi?id=563846 to update your patch
queue.

mozilla-central and mozilla-inbound are now open again.

Cheers,
--
Ehsan
<http://ehsanakhgari.org/>


On Thu, Oct 13, 2011 at 5:15 PM, Ehsan Akhgari <[hidden email]>wrote:

> Now that bug 690864 is WONTFIXed, this is on schedule again to happen on
> Monday, Oct 17 at 8:00AM PST.  mozilla-central and mozilla-inbound will be
> closed while we get green runs from this.  Please let me know if you have
> any concerns about this before then.
>
> Thanks!
> --
> Ehsan
> <http://ehsanakhgari.org/>
>
>
>
> On Fri, Sep 30, 2011 at 5:26 PM, Ehsan Akhgari <[hidden email]>wrote:
>
>> I took it upon myself to finish the other half of Michael Wu's awesome
>> work this week: which is switching from PR_TRUE/PR_FALSE to true/false on
>> mozilla-central.  I've filed bug 690892 for this.
>>
>> If everything goes as planned, I'm planning to close mozilla-central and
>> mozilla-inbound on Monday, Oct 3 at 8:00AM PST, do a merge, and then land
>> the patch.  The trees will remain closed while we get green runs.
>>
>> I've written a script which people can use to unbitrot their patch queues
>> once this lands.  You can find it at
>> https://bugzilla.mozilla.org/attachment.cgi?id=563846.
>>
>> I will update here once the change has been landed successfully and the
>> trees have been reopened.
>>
>> Cheers,
>> --
>> Ehsan
>> <http://ehsanakhgari.org/>
>>
>
>
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning