Skip to content

Optimizations for the WritePdbInfo function#61

Closed
jack-pappas wants to merge 1 commit intodotnet:fsharp4from
jack-pappas:ilwrite-opts
Closed

Optimizations for the WritePdbInfo function#61
jack-pappas wants to merge 1 commit intodotnet:fsharp4from
jack-pappas:ilwrite-opts

Conversation

@jack-pappas
Copy link
Copy Markdown
Contributor

I made some basic optimizations to the WritePdbInfo function in an attempt to speed it up; mostly, I've removed some closure allocations (using for loops instead of .iter functions) and changed an unnecessary use of a ref cell to use a normal variable binding instead.

Overall compilation performance seems to be only slightly improved, though I've only tested on my development machine. The benefit may be greater for machines with slower CPU and/or memory (e.g., CI servers running on VMs).

@latkin
Copy link
Copy Markdown
Contributor

latkin commented Jan 19, 2015

Thanks Jack. Do you have any reproducible measurements/tests that show the benefit of these changes?

…d intermediate data structure allocations.

Removed some unnecessary uses of ref cells.
@dsyme
Copy link
Copy Markdown
Contributor

dsyme commented Jan 19, 2015

@jack-pappas - yes, this would need perf figures

@latkin and I may need to write up some notes about what constitutes a minimum-bar for a compiler perf improvement to be accepted - I think minimally

  • Reliable, reproducible > 1% perf improvement. See also my other comment about automated CI compiler perf testing - if we could get that in place then we'd see the effect more easily.
  • Very, very clear there is absolutely no chance of a regression in logic

Without the demonstrated perf improvement a PR couldn't be accepted (and would be closed pending further information), hence the need for CI to make it cheap to reproduce these.

BTW I notice some of the code changes are just cleanup - you could resubmit those as a separate checkin labelled "cleanup" - it is very, very helpful to separate code cleanup from perf improvements. Again @latkin and I may need to write notes on what constitutes an acceptable "cleanup" checkin and at what stage.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

commented code

@latkin
Copy link
Copy Markdown
Contributor

latkin commented Jan 23, 2015

Thank you for the submission. Per the contribution guidelines, perf optimizations should include tests and quantitative analysis showing reliable gains from the change, which others can reproduce. Please re-open when tests and analysis are in place.

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.

4 participants