Skip to content

Ready for review#124

Merged
idontgetoutmuch merged 3 commits intointerface-to-performancefrom
release-notes-new
May 11, 2020
Merged

Ready for review#124
idontgetoutmuch merged 3 commits intointerface-to-performancefrom
release-notes-new

Conversation

@idontgetoutmuch
Copy link
Copy Markdown
Owner

No description provided.

Copy link
Copy Markdown
Collaborator

@curiousleo curiousleo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for doing this! I find claims of performance improvements much more credible when a lower bound is given too. In this case the lower bound is really respectable, so let's include it!

CHANGELOG.md Outdated
3. Monadic adapters for pure generators (providing a uniform monadic
interface to pure and monadic generators).
4. Faster by more x10 (depending on the type) - see below for benchmarks.
4. Faster by more x1000 (depending on the type) - see below for benchmarks.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super-nit: I tend to be skeptical of claims like "better by up to ". In this case, even the least improved generator is impressive, so I think we should mention it. i.e. "40 to 1000 times faster pseudo-random value generation". (I think 40 is about right, but worth checking again).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the minimum is INTEGER which is only 13% faster - the next lowest is x18 for types with 8 bits - I think if this is too heavily caveated you lose the message

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. No need to dwell on this too much. The other alternative that comes to mind would be e.g. "Faster generation for all implemented types, by up to x1000 for fixed-width integral types".

But if you think that's too much caveating, I'm fine with keeping this as it is.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something vague, but still accurate with a lower bound, along the lines: "Performance improvements all across the board with generation of most types being faster on the orders of x100 to x1000"

CHANGELOG.md Outdated
Here are some benchmarks run on a 3.1 GHz Intel Core i7. The full
benchmarks can be run using e.g. `stack bench`. The benchmarks are
measured in milliseconds per 100,000 generations. In some cases, the
performance is over x1000 times better.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here - I give much more weight to such claims personally when a lower bound is also given.

CHANGELOG.md Outdated
3. Monadic adapters for pure generators (providing a uniform monadic
interface to pure and monadic generators).
4. Faster by more x1000 (depending on the type) - see below for benchmarks.
4. Faster in all cases bar one by more than x18 (N.B. x18 not 18%) and
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean: bar one by? Was that intended or am I missing a piece of English vocabulary?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bar means except probably more common in the North - the exception is Integer - I will replace bar by except

There's a famous song: On Ilkley Moor bar t'hat (on Ilkley Moor without a hat) - actually it's probably not that famous - my children wouldn't know it.

@curiousleo
Copy link
Copy Markdown
Collaborator

Looking at the whole file, I just noticed this: I've seen a bunch of projects adding new releases to the top of the changelog rather than at the bottom. I guess this way the most recent = relevant changes are visible immediately. The most recent previous release, 1.1, is at the top. In the interest of consistency, I suggest moving the 1.2 release notes to the beginning of the file.

@idontgetoutmuch
Copy link
Copy Markdown
Owner Author

Looking at the whole file, I just noticed this: I've seen a bunch of projects adding new releases to the top of the changelog rather than at the bottom. I guess this way the most recent = relevant changes are visible immediately. The most recent previous release, 1.1, is at the top. In the interest of consistency, I suggest moving the 1.2 release notes to the beginning of the file.

Excellent idea!

Copy link
Copy Markdown
Collaborator

@curiousleo curiousleo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@idontgetoutmuch idontgetoutmuch merged commit 31f9df2 into interface-to-performance May 11, 2020
curiousleo pushed a commit that referenced this pull request May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants