darcs

Patch 1406 refactor breakAfterNthNewline and breakBeforeNthNewline

Title refactor breakAfterNthNewline and breakBeforeNthNewline
Superseder Nosy List gh
Related Issues
Status accepted Assigned To
Milestone 2.10.2

Created on 2015-11-05.23:30:48 by gh, last changed 2015-11-07.18:48:31 by gh.

Files
File name Status Uploaded Type Edit Remove
patch-preview.txt gh, 2015-11-05.23:30:47 text/x-darcs-patch
refactor-breakafternthnewline-and-breakbeforenthnewline.dpatch gh, 2015-11-05.23:30:47 application/x-darcs-patch
unnamed gh, 2015-11-05.23:30:47 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg18830 (view) Author: gh Date: 2015-11-05.23:30:47
I've compared both functions with their original version using
QuickCheck. Also the testsuite passes.

1 patch for repository http://darcs.net:

patch d36c66297d1d8da30a84c7e1c62b760c7fb63548
Author: Guillaume Hoffmann <guillaumh@gmail.com>
Date:   Thu Nov  5 20:37:50 ART 2015
  * refactor breakAfterNthNewline and breakBeforeNthNewline
  Profiling shows that breakBeforeNthNewline is faster and
  uses less memory (for instance with "darcs diff --last=2000").
Attachments
msg18831 (view) Author: gh Date: 2015-11-06.14:36:49
Some measurings with profiling enabled, with the command "darcs diff
--last=2000 +RTS -prof". This accounts only for the change of
breakBeforeNthNewline.

Before the patch:

1.
total time  =       13.74 secs   (13744 ticks @ 1000 us, 1 processor)
total alloc = 15,524,518,472 bytes  (excludes profiling overheads)

2.
total time  =        9.73 secs   (9726 ticks @ 1000 us, 1 processor)
total alloc = 15,523,474,664 bytes  (excludes profiling overheads)

3.
total time  =        9.63 secs   (9631 ticks @ 1000 us, 1 processor)
total alloc = 15,523,645,368 bytes  (excludes profiling overheads)

After:

1.
total time  =        7.11 secs   (7106 ticks @ 1000 us, 1 processor)
total alloc = 5,452,901,552 bytes  (excludes profiling overheads)

2.
total time  =        7.12 secs   (7115 ticks @ 1000 us, 1 processor)
total alloc = 5,447,844,184 bytes  (excludes profiling overheads)

3.
total time  =        7.18 secs   (7184 ticks @ 1000 us, 1 processor)
total alloc = 5,447,981,704 bytes  (excludes profiling overheads)


Profiling graphs ( with "darcs diff --last=2000 +RTS -h -i0.1" ) show
the same difference (from ~ 350 MBytes used to ~225 MBytes used).

Now for breakAfterNthNewline I don't have figures, it would be nice to
have a read-only command that uses this function a lot (unlike amend for
instance).
msg18832 (view) Author: gh Date: 2015-11-06.14:56:20
In fact after reading the module Darcs.Patch.Prim.V1.Apply, both
functions are used by "darcs diff", so it's good news.
msg18833 (view) Author: gh Date: 2015-11-06.17:13:02
After intensive quickcheck'ing (darcs-test --shell=no -q 10000) without
problem, I'm screening the bundle.

Also it applies cleanly to branch 2.10 so I would want to have it into
2.10.2.
msg18837 (view) Author: ganesh Date: 2015-11-07.00:06:01
I'm convinced by the QuickCheck results.

Am I right that the 'nth' newline is actually counted differently 
for the two functions, i.e. the output of `breakAfterNthNewline n` 
and `breakBeforeNthNewline n` don't just differ from each other by a 
single newline? if so it would be worth a comment to that effect.
msg18839 (view) Author: gh Date: 2015-11-07.18:48:31
Yes you're right.

I believe the whole applyHunks/applyHunkLines business in
Darcs.Patch.Prim.Apply (the only places that call
break{After,Before}NthNewline) should be documented, commented, and
probably refactored. breakAfterNthNewline and breakBeforeNthNewline may
end up moved to that module too.

For now, I wanted to tackle a low hanging fruit that weighted on the
memory consumption of "darcs diff". I'm opening a ticket for refactoring
Darcs.Patch.Prim.Apply .
History
Date User Action Args
2015-11-05 23:30:48ghcreate
2015-11-06 14:36:50ghsetmessages: + msg18831
2015-11-06 14:56:21ghsetmessages: + msg18832
2015-11-06 17:13:02ghsetstatus: needs-screening -> needs-review
messages: + msg18833
milestone: 2.10.2
2015-11-07 00:06:01ganeshsetstatus: needs-review -> accepted-pending-tests
messages: + msg18837
2015-11-07 12:23:48ganeshsetstatus: accepted-pending-tests -> accepted
2015-11-07 18:48:31ghsetmessages: + msg18839