Skip to content

ipld/unixfs/io: MaxLinks and MaxHAMTFanout for directories #897

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

Closed
wants to merge 4 commits into from

Conversation

hsanjuan
Copy link
Contributor

@hsanjuan hsanjuan commented Mar 25, 2025

This PR paves the way to support controlling and limiting the max number of links a directory can have by supporting option-based limitations on the unixfs directory implementation.

The result is that we can set a maximum number of links in Dynamic folders that will trigger conversion to HAMT, which has in turn it's own Fanout option.

This PR has a follow up at #906 .

@hsanjuan hsanjuan self-assigned this Mar 25, 2025
@hsanjuan hsanjuan requested a review from a team as a code owner March 25, 2025 22:47
@hsanjuan
Copy link
Contributor Author

(This is a supporting PR to being able to set dag-width in Kubo).

@hsanjuan hsanjuan force-pushed the feat/max-links-in-directories branch 2 times, most recently from 2a0a3f5 to fb96ebb Compare March 26, 2025 15:06
@hsanjuan hsanjuan changed the title ipld/unixfs/io: add WithMaxLinks() to directories ipld/unixfs/io: better initialization and options for Directories Mar 26, 2025
@hsanjuan hsanjuan force-pushed the feat/max-links-in-directories branch 2 times, most recently from 288fddd to 596680d Compare March 26, 2025 15:13
Copy link
Contributor

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

Logic all looks good, tests too. Could only find some trivial code nits.

lidel
lidel previously requested changes Mar 28, 2025
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Slight concern that WithMaxLinks alone does not allow users to hardcode current implicit defaults in this and other implementations, which is 174 or 1024 for Basic and 256 for HAMT fanout – comment inline.

WithMaxWidth()
--------------

The new option sets effectively a maximum width to the
directories.

Currently, on a dynamic directory, the switching to-from Basic to HAMT is
controlled by HAMTShardingSize, and the width of the HAMT shards by DefaultShardWidth.

When WithMaxLinks() is specified, the switching is additionally controlled by
the number of links exceeding the limit. In that case, MaxLinks is used as ShardWidth.

The directory can only be converted back to BasicDirectory when the total
number of links is below the limit.

Backwards compatibility is kept and tests have been added.

Note that when setting MaxLinks to a high number, it is possible that we still
suffer automatic conversion to HAMT before hitting MaxLinks, if the estimated
directory size is above 256KiB (as before).

WithStat()
----------

Allows to set Stat on a new directory.

WithCidBuilder()
---------------

Allows to set the CidBuilder on a new directory.
@hsanjuan hsanjuan force-pushed the feat/max-links-in-directories branch from 596680d to 562f0e5 Compare March 31, 2025 13:17
@hsanjuan hsanjuan requested a review from lidel April 4, 2025 15:48
@hsanjuan hsanjuan dismissed lidel’s stale review April 4, 2025 16:41

New review requested. Changes addressed.

@hsanjuan hsanjuan changed the title ipld/unixfs/io: better initialization and options for Directories ipld/unixfs/io: MaxLinks and MaxHAMTFanout for directories Apr 7, 2025
@hsanjuan
Copy link
Contributor Author

hsanjuan commented Apr 7, 2025

Let's continue on #906, as it has additionally touched the unixfs part a bit.

@hsanjuan hsanjuan closed this Apr 7, 2025
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