Implement -find-nonatomic-writes filter #9
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This works well for me: normal ghcfixed ghcNote the difference in the fixed one: It has no |
|
Tangential, but very interesting, is that tracing the new GHC is almost 10x faster (2 seconds vs 17 seconds). Using -90.73 4.769502 1 3552745 28 ptrace
+54.17 0.560906 1 417026 28 ptraceBecause we use By printing counts of bytes peeked, we investigated which syscalls are responsible for the difference. It turns out it's On older GHCs, access patterns look like this: and for the new one: Note how the old GHC This suggests that the new one is somehow much smarter at getting the relevant data out. We also checked which exact program it is that does the reading; that's What could create this difference? One thing I know is that my new-GHC build may be a It seems worth investigating this to know what change/difference makes for this big difference in IO and thus compiler build performance. CC @bgamari Edit: I filed GHC issue https://gitlab.haskell.org/ghc/ghc/issues/16515 for this. |
nh2
requested changes
Mar 28, 2019
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @qrilka)
src/System/Hatrace.hs, line 980 at r1 (raw file):
| FileWrite | FileClose | FileRename ByteString
Haddock if it's the old or new name
src/System/Hatrace.hs, line 988 at r1 (raw file):
-- * only calls to `write` are currently used as a marker for writes and syscalls -- `pwrite`, `writev`, `pwritev` are not taken into account fileWritesConduit :: (MonadIO m) => ConduitT (CPid, TraceEvent) (FilePath, FileWriteEvent) m ()
Add nice explanation
src/System/Hatrace.hs, line 1019 at r1 (raw file):
yieldFdEvent pid fd FileClose DetailedSyscallEnter_rename SyscallEnterDetails_rename { oldpathBS, newpathBS } -> do path <- liftIO $ resolveToPidCwd pid (T.unpack $ T.decodeUtf8 oldpathBS)
decodeUtf8 is partial and doens't have HasCallStack yet; add wrapper
src/System/Hatrace.hs, line 1032 at r1 (raw file):
yield (path, event) o_WRONLY = 0o00000001 o_RDWR = 0o00000002
Link some source
src/System/Hatrace.hs, line 1067 at r1 (raw file):
checkWrites' (FileWrite:es) = checkWrites' es checkWrites' (FileClose:es) = checkRename es checkWrites' (e: _) = unexpected "FileClose or FileWrite" e
case for better readability? Nicer tnames to reflect semantics?
src/System/Hatrace.hs, line 1068 at r1 (raw file):
checkWrites' (FileClose:es) = checkRename es checkWrites' (e: _) = unexpected "FileClose or FileWrite" e checkRename (FileRename path:es) =
Explain this; perhaps :for readability
src/System/Hatrace.hs, line 1088 at r1 (raw file):
extract <$> (fileWritesConduit .| foldlC collectWrite mempty) where collectWrite m (fp, e) = Map.alter (Just . maybe [e] (e:)) fp m
I recommend some types on the helper functions
src/System/Hatrace.hs, line 1093 at r1 (raw file):
partitionEithers . map (analyzeWrites' . second reverse) $ Map.toList m in Map.fromList noRenames <> Map.fromList (map (second AtomicWrite) renames) analyzeWrites' (src, es) = case analyzeWrites es of
Explain a bit
src/System/Hatrace.hs, line 1094 at r1 (raw file):
in Map.fromList noRenames <> Map.fromList (map (second AtomicWrite) renames) analyzeWrites' (src, es) = case analyzeWrites es of AtomicWrite target -> Right (target, src)
Terminology rename() further up, then here AtomicWrite, and then renames again -- warrants comment
nh2
reviewed
Mar 28, 2019
There was a problem hiding this comment.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @qrilka)
src/System/Hatrace.hs, line 980 at r1 (raw file):
Previously, nh2 (Niklas Hambüchen) wrote…
Haddock if it's the old or new name
Done
src/System/Hatrace.hs, line 1019 at r1 (raw file):
Previously, nh2 (Niklas Hambüchen) wrote…
decodeUtf8is partial and doens't haveHasCallStackyet; add wrapper
Done
|
Here's the output of the new tool on Click to expand outputsTo be analysed in and after commercialhaskell/stack#4559 (comment). |
4 tasks
Using this filtering on other syscalls requires tracking file open modes which complicate the code
qrilka
force-pushed
the
nonatomic-writes-filter
branch
from
88f7ca1
to
d76f533
Compare
qrilka
commented
Mar 28, 2019
There was a problem hiding this comment.
Reviewable status: 0 of 5 files reviewed, 6 unresolved discussions (waiting on @nh2)
src/System/Hatrace.hs, line 988 at r1 (raw file):
Previously, nh2 (Niklas Hambüchen) wrote…
Add nice explanation
Done.
src/System/Hatrace.hs, line 1032 at r1 (raw file):
Previously, nh2 (Niklas Hambüchen) wrote…
Link some source
removed in the end
src/System/Hatrace.hs, line 1068 at r1 (raw file):
Previously, nh2 (Niklas Hambüchen) wrote…
Explain this; perhaps
:for readability
Done.
src/System/Hatrace.hs, line 1088 at r1 (raw file):
Previously, nh2 (Niklas Hambüchen) wrote…
I recommend some types on the helper functions
Done.
src/System/Hatrace.hs, line 1093 at r1 (raw file):
Previously, nh2 (Niklas Hambüchen) wrote…
Explain a bit
Done.
src/System/Hatrace.hs, line 1094 at r1 (raw file):
Previously, nh2 (Niklas Hambüchen) wrote…
Terminology
rename()further up, then hereAtomicWrite, and thenrenamesagain -- warrants comment
Done.
qrilka
commented
Mar 29, 2019
There was a problem hiding this comment.
Reviewable status: 0 of 5 files reviewed, 6 unresolved discussions (waiting on @nh2)
src/System/Hatrace.hs, line 1032 at r1 (raw file):
Previously, qrilka (Kirill Zaborsky) wrote…
removed in the end
Done.
nh2
approved these changes
Mar 29, 2019
There was a problem hiding this comment.
Reviewed 1 of 5 files at r2, 3 of 4 files at r3, 1 of 1 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @qrilka)
src/System/Hatrace.hs, line 1007 at r5 (raw file):
deriving (Eq, Ord, Show) -- | Uses raw trace events to produces more focused events aimed at analysing file writes.
typo produce
src/System/Hatrace.hs, line 1095 at r5 (raw file):
unexpected : _ -> unexpectedEvent "FileClose or FileWrite" unexpected -- when it happens that a path gets more than 1 sequence open-write-close -- for it we need to check whethere there was a `rename` after the 1st one
typo whether
qrilka
force-pushed
the
nonatomic-writes-filter
branch
from
d76f533
to
73c4f0d
Compare
nh2
approved these changes
Apr 1, 2019
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6.
Reviewable status:complete! all files reviewed, all discussions resolved
|
I filed GHC issue https://gitlab.haskell.org/ghc/ghc/issues/16515 for the investigation of GHC linker performance. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
stack run -- hatrace --find-nonatomic-writes stack testonhatraceitself ended up withtest failure and many nonatomically written files
This change is