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

Faster catchup #1206

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Faster catchup #1206

wants to merge 24 commits into from

Conversation

larskuhtz
Copy link
Contributor

No description provided.

@larskuhtz larskuhtz changed the title Lars/batches Faster catchup Mar 1, 2021
@larskuhtz larskuhtz marked this pull request as ready for review May 11, 2022 19:51
@larskuhtz larskuhtz requested a review from edmundnoble May 12, 2022 14:44
@larskuhtz larskuhtz requested a review from jwiegley June 21, 2022 16:06
@@ -126,6 +126,7 @@ import qualified Data.Text as T
import Data.Word

import GHC.Generics (Generic)
import GHC.Stack
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the call stack still needed, or was that just for debugging?

@@ -210,7 +210,8 @@ cutMap :: Getter Cut (HM.HashMap ChainId BlockHeader)
cutMap = cutHeaders

lookupCutM
:: MonadThrow m
:: HasCallStack
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above. Is this for debugging?

@@ -730,17 +733,20 @@ blockDiffStream :: MonadIO m => CutDb cas -> S.Stream (Of (Either BlockHeader Bl
blockDiffStream db = cutStreamToHeaderDiffStream db $ cutStream db

cutHashesToBlockHeaderMap
:: PayloadCas cas
:: forall cas
Copy link
Collaborator

Choose a reason for hiding this comment

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

This quantification should not be necessary; you aren't using the variable within the function anywhere that I can see.

Nothing -> casLookup cas payloadHash >>= \case
(Just !x) -> return $! payloadWithOutputsToPayloadData x
Nothing -> memo memoMap payloadHash $ \k ->
pullOrigin k maybeOrigin >>= \case
Nothing -> do
t <- queryPayloadTask k
pQueueInsert queue t
awaitTask t
head <$> awaitTask t -- FIXME
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be fixed before merging.

Nothing -- cursor
Nothing -- limit
(Just . int $ min (_blockHeight curHdr) trgHeight) -- minimum rank
-- FIXME guarantee that at least one block is returned
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fix :)

rs $ S.next >=> \case
Left _ -> return Nothing
Right (x, r) -> do
unless (_blockHash x == k) $ error "pullOrigin: assertion failed"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would guard do the same thing here?

memoBatchAsync tm@(TaskMap var) ks batchTask = do
modifyMVarMasked var $ \m -> do
let missing = filter (not . (`HM.member` m)) ks
t <- asyncWithUnmask $ \umask -> umask (batchTask missing)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why use WithUnmask here if you immediately unmask the entire block being passed? What is the benefit?

edmundnoble
edmundnoble previously approved these changes Dec 15, 2022
Comment on lines +799 to +800
-- this is somewhat expensive but only relevant during chaingraph
-- transitions
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this more or less free, now that the genesis headers are cached?

casLookup candidateStore payloadHash >>= \case
Just !x -> return x
Just !x -> do
logfun Info $ taskMsg "got payload from candidate store"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this maybe too much logging? Should this be Debug level?

return Nothing
pullOrigin k (Just origin) = do
pullOrigin k (Just origin) = flip catchAllSynchronous (\_ -> return Nothing) $ do
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this catchAllSynchronous for?

instance (Typeable k, Show k) => Exception (NoBatchResultException k)

-- | Schedule a task that computes a batch of task map entries, that can be
-- awaited as indiviual tasks.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/indiviual/individual

@@ -282,7 +324,10 @@ getBlockHeaderInternal headerStore payloadStore candidateHeaderCas candidatePayl
-- - task queue of P2P network
--
(maybeOrigin', header) <- casLookup candidateHeaderCas k' >>= \case
Just !x -> return (maybeOrigin, x)
Just !x -> do
logg Info $ taskMsg k
Copy link
Contributor

Choose a reason for hiding this comment

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

^ Ditto, maybe Debug?

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.

3 participants