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

Replace the unsafe getmodtime with safe posix calls #1778

Merged
merged 6 commits into from
May 8, 2021
Merged

Conversation

pepeiborra
Copy link
Collaborator

@pepeiborra pepeiborra commented Apr 29, 2021

We don't need the 2X faster but unsafe getmodtime anymore since GetModificationTime is no longer called O(N) times but only O(FOI) times, where N is the number of known targets and FOI is the number of files of interest.

The main problem with the internal implementation of getModTime is that it's not cross platform. The unix package implements support for more platforms: https://github.com/haskell/unix/blob/master/System/Posix/Files/Common.hsc#L344-L365

This change switches to the safer unix implementation, which provides nanosecond resolution in systems that support it.

@pepeiborra pepeiborra force-pushed the getmodtime branch 8 times, most recently from 108ffbc to 1d82903 Compare May 2, 2021 17:41
@pepeiborra pepeiborra marked this pull request as ready for review May 2, 2021 19:45
@pepeiborra pepeiborra requested a review from ndmitchell May 2, 2021 19:45
Copy link
Collaborator

@ndmitchell ndmitchell left a comment

Choose a reason for hiding this comment

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

Getting rid of custom unsafe code seems good. But given this is no longer a bottleneck, why not go the full way and use https://hackage.haskell.org/package/directory-1.3.6.1/docs/System-Directory.html#v:getModificationTime on both Windows and Linux? For editors which don't support subscriptions, I guess this code path will still be very hot - but are we assuming those editors are going to have a bad time anyway? And I guess it doesn't include Vim/Emacs/VS Code?

ghcide/test/exe/Main.hs Show resolved Hide resolved
@pepeiborra
Copy link
Collaborator Author

Getting rid of custom unsafe code seems good. But given this is no longer a bottleneck, why not go the full way and use https://hackage.haskell.org/package/directory-1.3.6.1/docs/System-Directory.html#v:getModificationTime on both Windows and Linux? For editors which don't support subscriptions, I guess this code path will still be very hot - but are we assuming those editors are going to have a bad time anyway? And I guess it doesn't include Vim/Emacs/VS Code?

I want to fix the unsafe code but I don't want to give up all the performance, specially in large projects with thousands of files, given that we still read timestamps at least once for every file (or multiple times if the client doesn't support file watches) and multiple times for interface files.

But to be fair I haven't benchmarked - I have no idea if the cost of the conversions, one call to divMod, is that expensive.

@ndmitchell
Copy link
Collaborator

The code in Shake has been well tested and doesn't use any C - https://github.com/ndmitchell/shake/blob/master/src/Development/Shake/Internal/FileInfo.hs. Any reason we're not using that?

@pepeiborra
Copy link
Collaborator Author

The code in Shake has been well tested and doesn't use any C - https://github.com/ndmitchell/shake/blob/master/src/Development/Shake/Internal/FileInfo.hs. Any reason we're not using that?

It does use foreign import calls on the Windows path, and the Unix path is essentially the same as this PR with additional conversions that we don't need

@pepeiborra
Copy link
Collaborator Author

@ndmitchell do you have any other comments?

@pepeiborra pepeiborra changed the title Replace the unsafe getmodtime with the one from the posix package Replace the unsafe getmodtime with safe posix calls May 3, 2021
We don't need the 2X faster but unsafe getmodtime anymore
since GetModificationTime is not called O(N) anymore,
but only O(FOI) times, where N is the number
of known targets and FOI is the number of files of interest
@pepeiborra pepeiborra added the merge me Label to trigger pull request merge label May 8, 2021
@pepeiborra pepeiborra mentioned this pull request May 8, 2021
1 task
@mergify mergify bot merged commit 50ee7fa into master May 8, 2021
michaelpj added a commit that referenced this pull request Jun 10, 2023
Usage was removed in #1778.
@michaelpj michaelpj mentioned this pull request Jun 10, 2023
mergify bot added a commit that referenced this pull request Jun 10, 2023
Usage was removed in #1778.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants