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

Fix profiling when IO_Fork from the IO package is used #2908

Merged
merged 1 commit into from
Oct 9, 2018

Conversation

ChrisJefferson
Copy link
Contributor

Many apologises for the multiple PRs.

After further thought #2907 is both (a) too complicated, and (b) doesn't even do what I needed (it makes too many files). This adds a tiny part of that PR. I will then make a seperate PR to IO, which calls this function when in the child part of an IO_fork.

While this will tie profiling and IO_fork more tightly, I think that's the right option, as the forks in GAP never make processes which can continue to run GAP.

@ChrisJefferson ChrisJefferson added topic: kernel release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Oct 9, 2018
doc/ref/debug.xml Outdated Show resolved Hide resolved
src/profile.c Outdated
{
HashLock(&profileState);
if (profileState_Active) {
char filenamecpy[GAP_PATH_MAX + 20];
Copy link
Member

Choose a reason for hiding this comment

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

This is not a blocking comment, but still: Why the +20? That seems to defeat the purpose of GAP_PATH_MAX. If we are concerned the buffer might not be big enough (a quite reasonable concern, although hopefully in practice rarely of relevance), well, then we should avoid hard coded buffer sizes at all everywhere. OTOH, if we accept that we'll only support paths up to a certain length, then why suddenly make an exception here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because otherwise I get a warning from gcc complaining my snprintf could overflow it's buffer (which seems like a stupid warning, as isn't that the point of snprintf?) I couldn't figure out how to disable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, I accept that's stupid. I've now done it "properly", without having any warnings.

@codecov
Copy link

codecov bot commented Oct 9, 2018

Codecov Report

Merging #2908 into master will decrease coverage by <.01%.
The diff coverage is 10%.

@@            Coverage Diff             @@
##           master    #2908      +/-   ##
==========================================
- Coverage   83.74%   83.74%   -0.01%     
==========================================
  Files         681      680       -1     
  Lines      346547   346275     -272     
==========================================
- Hits       290228   289973     -255     
+ Misses      56319    56302      -17
Impacted Files Coverage Δ
src/profile.c 43.47% <10%> (-1%) ⬇️
lib/init.g 81.13% <0%> (-0.72%) ⬇️
src/integer.c 97.88% <0%> (-0.18%) ⬇️
src/gap.c 84.34% <0%> (-0.1%) ⬇️
lib/record.gd 100% <0%> (ø) ⬆️
lib/string.gd 100% <0%> (ø) ⬆️
src/julia_gc.c
src/stats.c 95.47% <0%> (+0.19%) ⬆️
src/gasman.c 88.36% <0%> (+0.43%) ⬆️
src/weakptr.c 99.2% <0%> (+0.69%) ⬆️
... and 2 more

@ChrisJefferson ChrisJefferson merged commit a8db247 into gap-system:master Oct 9, 2018
@ChrisJefferson ChrisJefferson deleted the min-fork-fix branch January 20, 2019 13:38
@DominikBernhardt DominikBernhardt changed the title Small fork improvement Fix profiling when IO_Fork from the IO package is used Aug 22, 2019
@DominikBernhardt DominikBernhardt added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 22, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants