Skip to content

Run downloading, unpacking, and saving in parallel for serialized syncs#5593

Merged
aryairani merged 9 commits intotrunkfrom
syncv2/sorted
Mar 19, 2025
Merged

Run downloading, unpacking, and saving in parallel for serialized syncs#5593
aryairani merged 9 commits intotrunkfrom
syncv2/sorted

Conversation

@ChrisPenner
Copy link
Member

@ChrisPenner ChrisPenner commented Feb 26, 2025

Overview

Corresponding with unisoncomputing/share-api#41 ; this tweaks the download streams from a sorted source to split up downloading, unpacking, and validation/save into pipelines which run in parallel, this allows us to be saving to SQLite we're downloading things in parallel, rather than interleaving downloads with saving as we do on trunk at the moment.

Implementation notes

  • Puts a TBMQueue in between each of 3 different conduit pipelines, then run the pipelines in parallel using UnliftIO.Conc.
  • The progress indicator now shows progress of each portion independently.

Test coverage

  • Tested against local share

@ChrisPenner ChrisPenner marked this pull request as ready for review March 11, 2025 18:59
@aryairani
Copy link
Contributor

Hey this is great, I finally got to test it.

I had 4 comments:

  1. When I clone @unison/base in a new codebase, between "Identifying missing entities" but before downloading begins, I have about 5 seconds of waiting with no feedback. What's happening during that time, is it just waiting for the server's response?
image
  1. For some reason, there's a leading 0 showing in the Downloaded field
image
  1. Suggest renaming Saved to Imported, and right-align them on the :.

I was going to suggest adding a percentage field for "Saved", but I guess it's a percentage of "Downloaded", but Downloaded keeps changing and we don't know in advance what the total will be. (Or do we?)

pure $
Text.unlines
[ "",
" ⬇️ Downloaded: " <> tShow unpacked <> maybe "" (\total -> " / " <> tShow total) total <> Monoid.whenM doneUnpacking " 🏁",
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 for a future server feature, or how do I trigger it?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a slot for the server to send the total if it knows what it is, but at the moment it doesn't ever send it because it would require enumerating the entire results set before starting the stream (and would slow everything down).

But if we ever support pre-computed release bundles then the server could provide the total here :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok sounds good

@ChrisPenner
Copy link
Member Author

ChrisPenner commented Mar 14, 2025

@aryairani which terminal emulator do you use btw? Mine looks different, though agree that right-aligning would be nice (iterm2) 😓
image

@aryairani
Copy link
Contributor

@aryairani which terminal emulator do you use btw? Mine looks different, though agree that right-aligning would be nice (iterm2) 😓 image

vs code:
Screenshot 2025-03-14 at 6 46 57 PM

terminal.app
image

huh, your terminal doesn't show a leading 0 either.

@ChrisPenner
Copy link
Member Author

ChrisPenner commented Mar 17, 2025

@aryairani ; Ahh, Looks like ⬇️ is U+2B07 U+FE0F, and 💾 is U+1F4BE;

Looks like iterm2 and terminal.app handle emoji codepoint width differently :'(

E.g. putting <emoji>123 in a file and catting it out gives:

terminal.app (don't mind the missing Nerdfont glyphs haha)
image

iTerm2
image

Looks like the current wisdom on this is "Massive headache, avoid if possible"

So I guess I gotta find a different emoji if we want things to line up 😓

@aryairani
Copy link
Contributor

aryairani commented Mar 17, 2025

@aryairani ; Ahh, Looks like ⬇️ is U+2B07 U+FE0F, and 💾 is U+1F4BE;

Looks like iterm2 and terminal.app handle emoji codepoint width differently :'(

E.g. putting <emoji>123 in a file and catting it out gives:

terminal.app (don't mind the missing Nerdfont glyphs haha) image

iTerm2 image

Looks like the current wisdom on this is "Massive headache, avoid if possible"

So I guess I gotta find a different emoji if we want things to line up 😓

Wow I'm surprised.

Terminal.app has a setting "Unicode East Asian Ambiguous chararacters are wide" which is disabled by default, but it doesn't seem to do anything. It would be nice if it wasn't buggy.

@ChrisPenner
Copy link
Member Author

@aryairani Ugh, looks like it gets even worse; AFAICT the console region lib we're using counts chars so it can surgically update only the chars in the region which have changed, and AFAICT it counts emojis as one column even though most emulators render them as two, which explains the weird 0's popping up :|

This is why we can't have nice things (a.k.a. emojis)

@ChrisPenner
Copy link
Member Author

Okay I just put the emoji AFTER all the alignment and numbers which sidesteps the issue, doesn't look quite as nice, but I think it's fine. If you'd rather I just remove them let me know :)

image

@ChrisPenner
Copy link
Member Author

Okay I've got it into some sort of reasonable output. I swear this progress bar is taking more time than the actual logic changed lol.

The reason for the long pause before the progress bar was because it was waiting for the stream header before starting the progress bar, since we didn't know whether the total was included or the sorting type before the stream actually starts, but initializing the stream takes time on the server because it's a complex query.

I refactored that a bit so the progress bar should start immediately (it'll still stay paused at 0 till things happen obviously) then will fill in the total and adjust to the stream type as needed.

@aryairani
Copy link
Contributor

This results in a 18% speedup on my M1 mac, over the serial version. 🎉

It's too bad the emojis at the beginning of the line didn't work, could you submit a bug report to the console region repo if you haven't already? At the end of the line, they kind of march across the screen as the length of the numbers increase.

image

Could you use the big number length to pad the smaller number into alignment, and then we'll merge it. 🙏

@hojberg
Copy link
Member

hojberg commented Mar 18, 2025

Alternatives to emojis, just regular characters:
Downloaded: ⤓ or ↓ or ▼
Imported: ✓ or ⬒

@ChrisPenner
Copy link
Member Author

I have officially given up on whimsy;
I removed all the emoji except the finish flag when it's done downloading.

image

@aryairani
Copy link
Contributor

😞

@aryairani aryairani merged commit 3851556 into trunk Mar 19, 2025
32 checks passed
@aryairani aryairani deleted the syncv2/sorted branch March 19, 2025 00:33
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