Conversation
Rebases the plugin/codec/ADL portions of ipfs#9016 ([Experiment] WASM IPLD Codecs and ADLs onto current master. The original PR was based on a 2022 codebase before the gateway was extracted to boxo; this commit ports only the kubo-appropriate parts. Changes: - plugin/ipld.go: add PluginIPLDADL interface for registering IPLD ADL reifiers - core/coreapi/coreapi.go: add LinkSystem() method and KnownReifiers map exposing an ipld-prime LinkSystem backed by the block service - plugin/loader/loader.go: wire PluginIPLDADL plugins through injectIPLDADLPlugin into coreapi.KnownReifiers - plugin/plugins/wasmipld/: new plugin enabling user-supplied WASM IPLD codecs and ADLs loaded via the Plugins config block (adapted import paths from go-ipfs to kubo, ioutil.ReadFile -> os.ReadFile) - plugin/loader/preload.go + preload_list: register wasmipld plugin - go.mod/go.sum: add github.com/aschmahmann/wasm-ipld/gobind, github.com/mitchellh/mapstructure, and github.com/bytecodealliance/wasmtime-go (bumped to v0.40.0 for macos-aarch64 prebuilt libwasmtime.a) NOT included (requires separate upstream work): - Gateway ?selector= query parameter rendering: the HTTP gateway has been extracted to github.com/ipfs/boxo/gateway. Per AGENTS.md, this is generic gateway protocol logic that belongs in boxo, not kubo, and adding a new gateway query parameter requires an IPIP for the HTTP Gateway spec (specs.ipfs.tech/http-gateways/) before it can ship. A follow-up should start with a boxo PR + IPIP. Builds: go build ./cmd/... ./core/... ./plugin/... Vet: go vet ./core/coreapi/... ./plugin/... Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> EOF )
When no plugin config is provided (the common case), Init returned early before setting registry, leaving it nil. Register then panicked dereferencing the nil registry. Move the initialization above the nil check so Register always sees an empty wasmRegistry. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
ipld.Batch divides the configured max batch size and node count by runtime.NumCPU(), so on a 20-core machine the 100MiB default becomes ~5MiB per parallel commit. It also requires decoding every block to an ipld.Node via CBOR, even though the CAR reader already returns blocks.Block and the blockservice only needs that. Replace ipld.NewBatch with direct accumulation of raw blocks.Block and flush via node.Blocks.AddBlocks(). This: - eliminates the NumCPU division of batch sizes - eliminates the unnecessary CBOR decode on every block - reduces write pressure (single synchronous flush vs NumCPU parallel) - removes the now-unused offline API construction This addresses the remaining write-amplification issues from ipfs#9678 that were not fixed by the WriteThrough default (PR ipfs#9721) or Pebble support (ipfs#10347). Also fix misleading comment: 100 << 20 is 100MiB, not 20MiB. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
This file contains hidden or 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
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.
Summary
ipld.NewBatchindag importwith direct accumulation of rawblocks.Blockand flush vianode.Blocks.AddBlocks()runtime.NumCPU()division of configured batch sizes (100MiB default becomes 5MiB per commit on 20 cores)blocks.Block, which is whatblockservice.AddBlocksexpects)cmdenv.GetApi+options.Api.Offline)100 << 20is 100MiB, not 20MiBContext
This addresses the remaining write-amplification issues from #9678 that were not already fixed:
Has()read amplification on writesThe
ipld.Batchcode ingo-ipld-formatdividesmaxSizeandmaxNodesbyruntime.NumCPU()(hardcodedparallelCommits = runtime.NumCPU()), so the user-configuredImport.BatchMaxSize(default 100MiB) andImport.BatchMaxNodes(default 128) are split across all CPU cores. On a 20-core machine, each parallel commit gets only ~5MiB / 6 nodes — far too small for efficient bulk import.Additionally,
ipld.Batch.Add()requiresipld.Node, forcingdag importto CBOR-decode every block from the CAR reader (which already providesblocks.Block) just to satisfy the type system. ThedagService.AddManyimplementation then immediately casts the nodes back toblocks.Blockforblockservice.AddBlocks. This double-decode showed prominently in CPU profiles (per @hsanjuan's analysis in #9678).By writing raw blocks directly to the blockservice, we:
The
blockDecoderis still used for the pin path, where decoding the root node is actually needed.Test plan
go build ./core/commands/dag/...passesgo vet ./core/commands/... ./config/...passesgofmt -lcleanTestDagImportCARv2— CARv2 import via HTTP APITestDagImportFastProvide(6 subtests) — fast-provide root/DAG with various flag/config combosTestDagImportPartialCAR— partial CAR importTestDagImportLocalOnlyImpliesNoPin— --local-only implies --pin-roots=falseTestDagImportLocalOnlyPinRootsConflict— --local-only + --pin-roots=true conflictTestBlockSizeBoundary/dag_import_and_export— 2MiB+1 block round-trip with --allow-big-blockTestCidBase— CID base conversion with dag importRefs #9678
Generated with Devin