tahoe-lafs/tahoe-lafs
tahoe-lafs
/
tahoe-lafs
2
0
Fork 0
Code Issues Pull Requests 23 Packages Projects Releases 1 Wiki Activity

Make immutable downloads less chatty #1251

Merged
itamarst merged 17 commits from 3946-less-chatty-downloads into master 2023-02-21 14:37:30 +00:00
Conversation 2 Commits 17 Files Changed 11 +107 -21
itamarst commented 2023-01-24 18:21:38 +00:00 (Migrated from github.com)
Copy Link

When latency is non-zero (i.e. anything not a LAN) this has huge improvement to download times, with both HTTP and Foolscap.

Downside: on Foolscap this increases CPU usage somewhat. Still seems worth it, presumably most Tahoe-LAFS interactions are not over LAN.

Fixes https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3946

When latency is non-zero (i.e. anything not a LAN) this has huge improvement to download times, with both HTTP and Foolscap. Downside: on Foolscap this increases CPU usage somewhat. Still seems worth it, presumably most Tahoe-LAFS interactions are not over LAN. Fixes https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3946
coveralls commented 2023-01-25 21:27:59 +00:00 (Migrated from github.com)
Copy Link

Coverage Status

Coverage: 94.838% (-0.02%) from 94.859% when pulling acc9cd2f9f on 3946-less-chatty-downloads into 54a6098d33 on master.

[![Coverage Status](https://coveralls.io/builds/57109793/badge)](https://coveralls.io/builds/57109793) Coverage: 94.838% (-0.02%) from 94.859% when pulling **acc9cd2f9f162409ef9896ab05b8c24efa69d1a9 on 3946-less-chatty-downloads** into **54a6098d33b951da9056c0a543ed0c0a2ef1d2d1 on master**.
exarkun (Migrated from github.com) reviewed 2023-01-31 15:23:13 +00:00
exarkun (Migrated from github.com) left a comment
Copy Link

Thanks. I think I have to learn some more stuff before I can give an approving review.

Thanks. I think I have to learn some more stuff before I can give an approving review.
integration/test_vectors.py
@ -39,2 +39,3 @@
await reconfigure(reactor, request, alice, (1, case.params.required, case.params.total), case.convergence)
await reconfigure(
reactor, request, alice, (1, case.params.required, case.params.total), case.convergence, case.segment_size)
exarkun (Migrated from github.com) commented 2023-01-31 14:28:35 +00:00
Copy Link

The other reconfigure caller in this module also needs to be updated to pass a value for max_segment_size. It has a value handy and it has strong feelings about what the segment size needs to be.

The other `reconfigure` caller in this module also needs to be updated to pass a value for `max_segment_size`. It has a value handy and it has strong feelings about what the segment size needs to be.
integration/util.py
exarkun (Migrated from github.com) commented 2023-01-31 14:43:40 +00:00
Copy Link

It looks like allmydata.interfaces.DEFAULT_MAX_SEGMENT_SIZE is undocumented.

I think this PR assumes that it is the segment size a client node will use when encoding CHK (aka "immutable") shares - unless the node is otherwise configured with the new config option being added.

In particular, it won't affect existing CHK shares and it won't affect SSK (SDMF/MDMF) (aka "mutable") shares.

If that matches your understanding, can you add a comment to the definition of DEFAULT_MAX_SEGMENT_SIZE?

It looks like `allmydata.interfaces.DEFAULT_MAX_SEGMENT_SIZE` is undocumented. I think this PR assumes that it is the segment size a client node will use when encoding CHK (aka "immutable") shares - unless the node is otherwise configured with the new config option being added. In particular, it won't affect existing CHK shares and it won't affect SSK (SDMF/MDMF) (aka "mutable") shares. If that matches your understanding, can you add a comment to the definition of `DEFAULT_MAX_SEGMENT_SIZE`?
src/allmydata/immutable/downloader/node.py
exarkun (Migrated from github.com) commented 2023-01-31 15:21:51 +00:00
Copy Link

Uhh this is so weird. Guess a value that is trivially readable from the front of any share and derive a bunch of real values from it -- then replace all those values with the real values after doing the trivial read -- supposedly as an optimization for Segmentation -- which cannot possibly work correctly if you guessed wrong? :/ :/ :/ :/

I guess I get to read some more of allmydata/immutable/downloader/ now...

Uhh this is so weird. Guess a value that is trivially readable from the front of any share and derive a bunch of real values from it -- then replace all those values with the real values after doing the trivial read -- supposedly as an optimization for `Segmentation` -- which cannot possibly work correctly if you guessed wrong? :/ :/ :/ :/ I guess I get to read some more of `allmydata/immutable/downloader/` now...
src/allmydata/test/test_system.py
exarkun (Migrated from github.com) commented 2023-01-31 15:22:58 +00:00
Copy Link

Perhaps the integration test suite is better suited to this kind of thing?

Perhaps the integration test suite is better suited to this kind of thing?
itamarst (Migrated from github.com) reviewed 2023-02-06 20:35:54 +00:00
integration/util.py
itamarst (Migrated from github.com) commented 2023-02-06 20:35:54 +00:00
Copy Link

That matches my understanding, yes.

That matches my understanding, yes.
itamarst (Migrated from github.com) reviewed 2023-02-06 20:36:31 +00:00
src/allmydata/immutable/downloader/node.py
itamarst (Migrated from github.com) commented 2023-02-06 20:36:31 +00:00
Copy Link

If it can't work if you guess wrong then this PR is no good :(

If it can't work if you guess wrong then this PR is no good :(
itamarst (Migrated from github.com) reviewed 2023-02-06 20:36:50 +00:00
src/allmydata/immutable/downloader/node.py
itamarst (Migrated from github.com) commented 2023-02-06 20:36:50 +00:00
Copy Link

But if you can get the segment value from trivial read... I should do that instead?

But if you can get the segment value from trivial read... I should do that instead?
itamarst (Migrated from github.com) reviewed 2023-02-06 20:37:52 +00:00
src/allmydata/test/test_system.py
itamarst (Migrated from github.com) commented 2023-02-06 20:37:52 +00:00
Copy Link

Hmhm. I was thinking I'd need to push the segment size through another N layers of abstraction but I guess with a config option then maybe not?

Hmhm. I was thinking I'd need to push the segment size through another N layers of abstraction but I guess with a config option then maybe not?
itamarst commented 2023-02-07 15:30:45 +00:00 (Migrated from github.com)
Copy Link

Note that the integration test failure with i2pd will be fixed when #1250 is merged.

Note that the integration test failure with i2pd will be fixed when #1250 is merged.
exarkun (Migrated from github.com) reviewed 2023-02-13 15:58:55 +00:00
src/allmydata/immutable/downloader/node.py
exarkun (Migrated from github.com) commented 2023-02-13 15:58:55 +00:00
Copy Link

It seems like incorrect guesses don't impair functionality. Maybe they make performance slightly worse or something (we could try to measure but I don't particularly want to at the moment).

So I think this part is fine as-is.

It seems like incorrect guesses don't impair functionality. Maybe they make performance slightly worse or something (we could try to measure but I don't particularly want to at the moment). So I think this part is fine as-is.
exarkun (Migrated from github.com) approved these changes 2023-02-13 16:00:23 +00:00
exarkun (Migrated from github.com) left a comment
Copy Link

This looks good to me (in particular, I can't see how it would break compatibility with existing clients or stored data - and the new integration test seems to support this conclusion pretty well).

This looks good to me (in particular, I can't see how it would break compatibility with existing clients or stored data - and the new integration test seems to support this conclusion pretty well).
Sign in to join this conversation.
Reviewers
No reviewers
exarkun
Labels
Clear labels   
Benchmarking and Performance

   
HTTP Storage Protocol

   
Nevow Removal

   
Python 3 Porting

   
not-for-merge
No Label
Benchmarking and Performance
HTTP Storage Protocol
Nevow Removal
Python 3 Porting
not-for-merge
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: tahoe-lafs/tahoe-lafs#1251
No description provided.
Delete Branch "3946-less-chatty-downloads"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?