Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove gprof support #2314

Merged
merged 5 commits into from Mar 16, 2019

Conversation

@mshinwell
Copy link
Contributor

commented Mar 12, 2019

Following a discussion on the core developers' list, which indicated that removing gprof support was a reasonable thing to do, this patch does just that.

The rationale is that there are better easy-to-use profilers out there now, such as perf for Linux and Instruments on macOS; and the gprof support has always been patchy across targets. We save a whole build of the runtime and simplify some other parts of the codebase by removing it.

I'm marking this as a debugging-prerequisite since a final decision is needed on this before we can proceed with #2292, which is directly needed for the debugging work.

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

Should this PR also delete tools/ocamlprof.ml?

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

I believe it should not: ocamlprof is a separate tool for instrumentation, unrelated to gprof as far as I know.

@stedolan

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

There is also a chunk of profiling-related stuff to be deleted in asmrun/i386.S and asmrun/arm.S. Grep for mcount.

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

Oh, OK, I thought ocamlprof was meant to be the implementation of ocamlcp.

(It should probably not block this PR, but at some point both the manual and the
man pages should be refreshed.)

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

Yeah, I am about to fix the manual. And in fact I don't think ocamlcp should be removed, so I'm resurrecting that. I've found various other things that need removing too -- another changeset coming soon.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

Ah, so in fact ocamloptp is for ocamlprof as well, so that should stay.

@mshinwell mshinwell force-pushed the mshinwell:no_gprof branch from 81eb4ee to a466efd Mar 12, 2019
@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

New version pushed.

@stedolan

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

This patch deletes profiling from ocamlc -config. Have you tested that this doesn't break package builds? Or is the intent to update any build systems with fragile -config parsers before 4.09? Alternatively, would you consider leaving profiling: false in ocamlc -config?

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

I did wonder about that. @dra27 Do you think we should leave the line in the -config output, always false?

@avsm

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Given it's early in the 4.09 release cycle, we could remove it from the -config output and see if anything breaks. It can always be restored subsequently if it is a problem.

@mshinwell mshinwell force-pushed the mshinwell:no_gprof branch from a466efd to 0484444 Mar 13, 2019
@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

@avsm I think I'm in favour of doing that.

@stedolan Since you've looked at this partially, would you be able to review the rest and approve? (I would like @xavierleroy 's signoff before we merge, however.)

@mshinwell mshinwell force-pushed the mshinwell:no_gprof branch from 0484444 to b5c144e Mar 13, 2019
Copy link
Contributor

left a comment

Code changes look good to me! There's a couple of other places where references to profiling should be removed, though:

  • driver/main_args.ml: mk_p has now-wrong text for the -p option
  • stdlib/Compflags: has rules for foo.p.cmx
  • stdlib/.depend: has .p.cmx dependencies. (It looks like you updated the .depend generator, so I think all you should need to do here is rerun make depend)
  • manual, manpages
@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

Thanks, I will check these. I thought I dealt with the manual though.

@stedolan

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

I don't see any changes to manual/ here?

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

I think I must have lost those somehow. I've done them again, there will be an update shortly.

@mshinwell mshinwell force-pushed the mshinwell:no_gprof branch from b5c144e to a6a39ce Mar 15, 2019
@mshinwell mshinwell requested a review from stedolan Mar 15, 2019
Copy link
Contributor

left a comment

I was surprised that stdlib/.depend got so much shorter. Turns out there were duplicate entries for all the cmo files because of how the profiling dependencies were generated!

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

@gasche Given that @xavierleroy probably hasn't had time to look at this, are you willing to confirm that this feature removal is ok?

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

I'm afraid I can't replace Xavier in terms of decision-making legitimacy. If you want "formal approval", maybe it's okay to wait an extra week? (I'll meet him on Wednesday/Tuesday so that's a worst-case timeline for an answer.)

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

Sure, we can wait.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

I had a quick look at the diff and it looks OK. I just started a round of precheck CI to make sure all architectures still compile and run.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

Precheck seems happy. Based on this, @stedolan's careful review, my own cursory check, and my pleasure at seeing so much boring code going away, I'm happy to merge this PR right now.

@xavierleroy xavierleroy merged commit 2cc1ea2 into ocaml:trunk Mar 16, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.