Quantcast

Re: Better perf for haddock.base, haddock.Cabal (f4aa998)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Better perf for haddock.base, haddock.Cabal (f4aa998)

Joachim Breitner-2
Hi,

Am Donnerstag, den 16.02.2017, 17:46 +0000 schrieb [hidden email]:

> commit f4aa9984790332a908e8b1321d00a839fb42c3ea
> Author: Simon Peyton Jones <[hidden email]>
> Date:   Thu Feb 16 17:44:58 2017 +0000
>
>     Better perf for haddock.base, haddock.Cabal
>     
>     I think this is due to
>       commit 6bab649bde653f13c15eba30d5007bef4a9a9d3a
>       Author: Simon Peyton Jones <[hidden email]>
>       Date:   Thu Feb 16 09:42:32 2017 +0000
>     
>           Improve checking of joins in Core Lint
>     
>     Improvement is around 5%.
no,
https://perf.haskell.org/ghc/#revision/fc9d152b058f21ab03986ea722d0c94688b9969f
clearly points to this commit:

    commit fc9d152b058f21ab03986ea722d0c94688b9969f
    Author: Simon Peyton Jones <    [hidden email]    >
    Date:   Thu Feb 16 09:41:55 2017 +0000

        Comments and tiny refactor only

    Which is not just a refactoring. If you look at the diff, e.g. at
    https://phabricator.haskell.org/rGHCfc9d152b058f21ab03986ea722d0c94688b9969f
you will see that after your change, the “one shot arguments according
to the environment” are no longer counted towards n_val_args in
fun_uds and is_exp. And it seems they should not! Is that something you
understand and can explain in a note?

I guess https://phabricator.haskell.org/D3089 was merged a bit
prematurely in that respect.¹

Greetings,
Joachim


¹ There is a workflow problem with Phab’s DR:
  * A creates a new DR.
  * B requests changes. DR now in state “revision needed”
  * C requests changes. DR still in state “revision needed”
  * A makes changes. DR now in state “needs review”
  * C looks at the changes and finds his concern addressed
    and accepts the revision. DR now in state “accepted”.
  * G comes along, sees a DR in state “accepted” and lands it.
  Problem: B did not have the chance to check the new revision.


 



--
Joachim “nomeata” Breitner
  [hidden email]https://www.joachim-breitner.de/
  XMPP: [hidden email] • OpenPGP-Key: 0xF0FBF51F
  Debian Developer: [hidden email]
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Better perf for haddock.base, haddock.Cabal (f4aa998)

Ben Gamari-2
Joachim Breitner <[hidden email]> writes:

> Hi,
>
...

>
> I guess https://phabricator.haskell.org/D3089 was merged a bit
> prematurely in that respect.¹
>
> Greetings,
> Joachim
>
>
> ¹ There is a workflow problem with Phab’s DR:
>   * A creates a new DR.
>   * B requests changes. DR now in state “revision needed”
>   * C requests changes. DR still in state “revision needed”
>   * A makes changes. DR now in state “needs review”
>   * C looks at the changes and finds his concern addressed
>     and accepts the revision. DR now in state “accepted”.
>   * G comes along, sees a DR in state “accepted” and lands it.
>   Problem: B did not have the chance to check the new revision.
>
Actually, the problem in this particular case was the Simon left
comments but didn't request changes. Had he done so the Diff wouldn't
have entered "accepted" state until he accepted. Sorry if I had been a
bit premature in merging this one.

While I understand why this is the case, it can be a bit unfortunate in
the case of an open-source project, where a drive-by reviewer might
leave a comment, the author makes the requested change, and the reviewer
never returns to accept the change. In this case the Diff remains in a
sort of limbo, even if someone else accepts it.

This is to some extent a social problem: In an ideal world reviewers
would continue to submit reviews until the patch is merged. However, in
the case of a project like GHC this is rarely the case. For this reason
I sometimes need to ping reviewers and explicitly ask them to accept
changes.

Cheers,

- Ben

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: DR workflows

Joachim Breitner-2
Hi

Am Donnerstag, den 16.02.2017, 17:01 -0500 schrieb Ben Gamari:

> > ¹ There is a workflow problem with Phab’s DR:
> >   * A creates a new DR.
> >   * B requests changes. DR now in state “revision needed”
> >   * C requests changes. DR still in state “revision needed”
> >   * A makes changes. DR now in state “needs review”
> >   * C looks at the changes and finds his concern addressed
> >     and accepts the revision. DR now in state “accepted”.
> >   * G comes along, sees a DR in state “accepted” and lands it.
> >   Problem: B did not have the chance to check the new revision.
> >
>
> Actually, the problem in this particular case was the Simon left
> comments but didn't request changes. Had he done so the Diff wouldn't
> have entered "accepted" state until he accepted.
Indeed! I thought I saw “Simon requested changes“ in this one, but my
memory was failing me here. So I partly retract my fussmaking here.


> While I understand why this is the case, it can be a bit unfortunate in
> the case of an open-source project, where a drive-by reviewer might
> leave a comment, the author makes the requested change, and the reviewer
> never returns to accept the change. In this case the Diff remains in a
> sort of limbo, even if someone else accepts it.

Would it not at least show up on Phab’s starting page, under “Ready to
Review” or some of the other categories. In the worst case, the author
can ping the reviewer here. I think it is fine that if someone requests
changes to expect him to follow up. The drive-by reviewer should leave
comments without requesting changes then.

> This is to some extent a social problem: In an ideal world reviewers
> would continue to submit reviews until the patch is merged. However, in
> the case of a project like GHC this is rarely the case. For this reason
> I sometimes need to ping reviewers and explicitly ask them to accept
> changes.

This is fine, I think.


What I sometimes miss as a reviewer is to indicate: “As far as I am
concerned, this looks good, but I did not review the whole thing”, for
example if one aspect of a change interacts with “my” code in GHC, and
I verified that this part is ok.

Greetings,
Joachim

--
Joachim “nomeata” Breitner
  [hidden email]https://www.joachim-breitner.de/
  XMPP: [hidden email] • OpenPGP-Key: 0xF0FBF51F
  Debian Developer: [hidden email]
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: Better perf for haddock.base, haddock.Cabal (f4aa998)

GHC - devs mailing list
In reply to this post by Joachim Breitner-2
| clearly points to this commit:
|
|     commit fc9d152b058f21ab03986ea722d0c94688b9969f
|     Author: Simon Peyton Jones <    [hidden email]    >
|     Date:   Thu Feb 16 09:41:55 2017 +0000
|
|         Comments and tiny refactor only

Crumbs!  You are right!

I have NO IDEA why this effect is so large.  I fixed the bug in occAnalApp
when I was auditing your changes, but I couldn't think of a situation in which
I would matter, and then I forgot all about it.

It's hard to add a comment.... both the call to mkOneOcc and isExpandableApp were designed for the number of args the function is *syntactically* applied to.  But the argOneShots needs how many it is *semantically* applied to, once.

But is_exp only matters if isRhsEnv is true; and in that case I think occ_one_shots is empty (see rhsCtxt); so I doubt the change to is_exp makes any difference at all.

It's a mystery.  Ideally one would find out why.  But life is short, and this is in the right direction.

I've added a note to the ticket, FWIW

Simon

| -----Original Message-----
| From: ghc-devs [mailto:[hidden email]] On Behalf Of Joachim
| Breitner
| Sent: 16 February 2017 20:41
| To: [hidden email]
| Subject: Re: Better perf for haddock.base, haddock.Cabal (f4aa998)
|
| Hi,
|
| Am Donnerstag, den 16.02.2017, 17:46 +0000 schrieb [hidden email]:
| > commit f4aa9984790332a908e8b1321d00a839fb42c3ea
| > Author: Simon Peyton Jones <[hidden email]>
| > Date:   Thu Feb 16 17:44:58 2017 +0000
| >
| >     Better perf for haddock.base, haddock.Cabal
| >
| >     I think this is due to
| >       commit 6bab649bde653f13c15eba30d5007bef4a9a9d3a
| >       Author: Simon Peyton Jones <[hidden email]>
| >       Date:   Thu Feb 16 09:42:32 2017 +0000
| >
| >           Improve checking of joins in Core Lint
| >
| >     Improvement is around 5%.
|
| no,
| https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fperf.has
| kell.org%2Fghc%2F%23revision%2Ffc9d152b058f21ab03986ea722d0c94688b9969f&d
| ata=02%7C01%7Csimonpj%40microsoft.com%7Caacdd3c960ea4e56ea6c08d456ac2c5d%
| 7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636228744888337816&sdata=vZW
| XXwyBzBcUMliVMtS9vUFmlQuBXOhciC9uzSm7aW0%3D&reserved=0
| clearly points to this commit:
|
|     commit fc9d152b058f21ab03986ea722d0c94688b9969f
|     Author: Simon Peyton Jones <    [hidden email]    >
|     Date:   Thu Feb 16 09:41:55 2017 +0000
|
|         Comments and tiny refactor only
|
|     Which is not just a refactoring. If you look at the diff, e.g. at
|
| https://phabricator.haskell.org/rGHCfc9d152b058f21ab03986ea722d0c94688b99
| 69f
| you will see that after your change, the “one shot arguments according to
| the environment” are no longer counted towards n_val_args in fun_uds and
| is_exp. And it seems they should not! Is that something you understand
| and can explain in a note?
|
| I guess https://phabricator.haskell.org/D3089 was merged a bit
| prematurely in that respect.¹
|
| Greetings,
| Joachim
|
|
| ¹ There is a workflow problem with Phab’s DR:
|   * A creates a new DR.
|   * B requests changes. DR now in state “revision needed”
|   * C requests changes. DR still in state “revision needed”
|   * A makes changes. DR now in state “needs review”
|   * C looks at the changes and finds his concern addressed
|     and accepts the revision. DR now in state “accepted”.
|   * G comes along, sees a DR in state “accepted” and lands it.
|   Problem: B did not have the chance to check the new revision.
|
|
|
|
|
|
| --
| Joachim “nomeata” Breitner
|   [hidden email]
| https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.joac
| him-
| breitner.de%2F&data=02%7C01%7Csimonpj%40microsoft.com%7Caacdd3c960ea4e56e
| a6c08d456ac2c5d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636228744888
| 337816&sdata=vsGnQwAfqS%2B0hfL0ZLwnDy87eNSBeLltJQXqPOadnXo%3D&reserved=0
|   XMPP: [hidden email] • OpenPGP-Key: 0xF0FBF51F
|   Debian Developer: [hidden email]
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Loading...