blockservice & exchange & bitswap: add non variadic NotifyNewBlock by Jorropo · Pull Request #242 · ipfs/boxo

Jorropo · 2023-03-30T16:48:48Z

Variadicts in go are just syntactic sugar around passing a slice, that means all go memory reachability rules apply, this force the compiler to heap allocate the variadic slice for virtual call, because the implementation is allowed to leak the slice (and go's interprocedural optimisations do not cover virtuals).

Passing a block without variadic will pass the itab either on the stack or decomposed through registers. Skipping having to allocate a slice.

codecov · 2023-03-30T17:07:09Z

Codecov Report

❌ Patch coverage is 77.55102% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.60%. Comparing base (e84eae1) to head (23f130e).
⚠️ Report is 179 commits behind head on main.

Files with missing lines	Patch %	Lines
exchange/providing/providing.go	40.00%	4 Missing and 2 partials ⚠️
bitswap/client/client.go	86.66%	2 Missing ⚠️
blockservice/blockservice.go	50.00%	1 Missing and 1 partial ⚠️
bitswap/bitswap.go	87.50%	1 Missing ⚠️

@@            Coverage Diff             @@
##             main     #242      +/-   ##
==========================================
- Coverage   61.63%   61.60%   -0.03%     
==========================================
  Files         254      254              
  Lines       31427    31469      +42     
==========================================
+ Hits        19370    19387      +17     
- Misses      10486    10507      +21     
- Partials     1571     1575       +4

Files with missing lines	Coverage Δ
...wap/client/internal/notifications/notifications.go	`90.10% <100.00%> (+2.75%)`	⬆️
bitswap/server/internal/decision/engine.go	`92.14% <ø> (ø)`
bitswap/server/server.go	`58.06% <100.00%> (+0.86%)`	⬆️
exchange/offline/offline.go	`88.88% <100.00%> (+1.01%)`	⬆️
bitswap/bitswap.go	`77.47% <87.50%> (+0.77%)`	⬆️
bitswap/client/client.go	`78.93% <86.66%> (+0.98%)`	⬆️
blockservice/blockservice.go	`78.00% <50.00%> (-0.23%)`	⬇️
exchange/providing/providing.go	`34.61% <40.00%> (-27.89%)`	⬇️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

exchange/interface.go

BigLep · 2023-03-30T17:49:44Z

@Jorropo : I don't see an issue linked to this PR. In the absence of this, can you share more on what's motivating this PR?

Jorropo · 2023-03-30T18:07:58Z

@BigLep the PR and Commit bodies:

Variadicts in go are just syntactic sugar around passing a slice, that means all go memory reachability rules apply, this force the compiler to heap allocate the variadic slice for virtual call, because the implementation is allowed to leak the slice (and go's interprocedural optimisations do not cover virtuals).
Passing a block without variadic will pass the itab either on the stack or decomposed through registers. Skipping having to allocate a slice.

I can add the explanation about keeping it in-line with our other exchange APIs.

This seems like a good usecase to try the idea @guseggert is cooking (some automated code migrator), that would add the NotifyNewBlock method (which redirects to NotifyNewBlocks) automatically if you run it.

hacdias

I don't have strong opinions on this. It seems a relatively small change that can bring memory benefits - although arguably low. If others agree, I'm fine to have it merged.

FYSA: I rebased the PR and added the updated exchange for the gateway examples.

BigLep · 2023-04-06T15:28:21Z

2023-04-06 conversation: will defer for now. Want to have clarity on how we handle breaking changes in Boxo.

gammazero · 2024-09-20T23:15:53Z

This is ready to merge. Let's decide if we want this or not.

lidel

Lgtm? As noted in https://github.com/ipfs/boxo/pull/242/files#r1153577309 this follows existing convention from other places (Put vs PutMany, GetBlock vs GetBlocks etc), and simplifies code slightly. If someone has custom implementation, should be trivial to migrate.

@aschmahmann any concerns?

Variadicts in go are just syntactic sugar around passing a slice, that means all go memory reachability rules apply, this force the compiler to heap allocate the variadic slice for virtual call, because the implementation is allowed to leak the slice (and go's interprocedural optimisations do not cover virtuals). Passing a block without variadic will pass the itab either on the stack or decomposed through registers. Skipping having to allocate a slice.

gammazero · 2024-12-14T00:55:08Z

CI fails when calling s.exchange.NotifyNewBlock(ctx, o) in blockservice.go here, but succeeds when calling s.exchange.NotifyNewBlocks(ctx, o). Ther should be no difference.

bitswap/bitswap.go

bitswap/server/server.go

gammazero · 2025-06-16T23:10:58Z

bitswap/server/server.go

+	// This is fine to do because if no calls are virtual then the compiler is able
+	// to see that the slice does not leak and the slice is stack allocated.


It is not clear whether this is true or not in the golang documentation.

gammazero · 2025-06-16T23:39:47Z

bitswap/client/client.go

+	// This is actually fine to do because no calls is virtual the compiler is able
+	// to see that the slice does not leak and the slice is stack allocated.


Escape analysis results seem to contradict this:

./client.go:432:7: leaking param content: bs ./client.go:432:34: leaking param: ctx ./client.go:432:55: leaking param: blk

gammazero · 2025-07-02T05:06:24Z

I do not think it is worth the extra complexity to have separate functions, one for a single block and one for multiple blocks.

Jorropo self-assigned this Mar 30, 2023

Jorropo requested a review from a team as a code owner March 30, 2023 16:48

Jorropo force-pushed the notify-new-block branch 3 times, most recently from 5344f5d to 4ff4bd2 Compare March 30, 2023 17:02

MichaelMure approved these changes Mar 30, 2023

View reviewed changes

aschmahmann reviewed Mar 30, 2023

View reviewed changes

exchange/interface.go Show resolved Hide resolved

BigLep mentioned this pull request Mar 30, 2023

[WIP] Frequently Asked Questions about this project #190

Closed

guseggert force-pushed the main branch from cb788ab to 5dee8e4 Compare March 31, 2023 14:34

hacdias force-pushed the main branch from 7c7aa8d to a3f1b66 Compare April 3, 2023 13:38

hacdias requested review from hacdias and lidel as code owners April 3, 2023 13:38

hacdias force-pushed the notify-new-block branch from 4ff4bd2 to 3af3c29 Compare April 5, 2023 08:28

hacdias approved these changes Apr 5, 2023

View reviewed changes

hacdias force-pushed the notify-new-block branch from 3af3c29 to 311cd1f Compare April 5, 2023 08:37

Jorropo removed their assignment Mar 4, 2024

gammazero force-pushed the notify-new-block branch from 311cd1f to 6a45c0e Compare September 20, 2024 21:54

gammazero added the need/maintainers-input Needs input from the current maintainer(s) label Sep 20, 2024

lidel reviewed Sep 27, 2024

View reviewed changes

Jorropo and others added 5 commits September 27, 2024 20:49

Update test interface

a76c8bd

remove unused struct

fb743c0

update changelog

70c1906

doc updates

730b9bd

gammazero force-pushed the notify-new-block branch from 73226d5 to 730b9bd Compare September 28, 2024 03:51

gammazero added 2 commits December 13, 2024 11:59

Merge branch 'main' into notify-new-block

781d93b

Merge branch 'main' into notify-new-block

29d92d0

Merge branch 'main' into notify-new-block

b0b4ed4

gammazero reviewed Jun 16, 2025

View reviewed changes

bitswap/bitswap.go Outdated Show resolved Hide resolved

gammazero reviewed Jun 16, 2025

View reviewed changes

bitswap/server/server.go Outdated Show resolved Hide resolved

gammazero reviewed Jun 16, 2025

View reviewed changes

bitswap/server/server.go Outdated Show resolved Hide resolved

gammazero added 3 commits June 16, 2025 16:00

Update bitswap/bitswap.go

b793b39

Update bitswap/server/server.go

59c1d84

Update bitswap/server/server.go

05e0093

gammazero reviewed Jun 16, 2025

View reviewed changes

gammazero added 3 commits June 16, 2025 14:52

do not call variadic for non-variadic

bd66825

Merge branch 'main' into notify-new-block

7f717bc

Supply missing NotifyNewBlock function

23f130e

gammazero closed this Jan 15, 2026

		// This is fine to do because if no calls are virtual then the compiler is able
		// to see that the slice does not leak and the slice is stack allocated.

		// This is actually fine to do because no calls is virtual the compiler is able
		// to see that the slice does not leak and the slice is stack allocated.

Conversation

Jorropo commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

BigLep commented Mar 30, 2023

Uh oh!

Jorropo commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hacdias left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BigLep commented Apr 6, 2023

Uh oh!

gammazero commented Sep 20, 2024

Uh oh!

lidel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gammazero commented Dec 14, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gammazero Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

gammazero Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

gammazero commented Jul 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Jorropo commented Mar 30, 2023 •

edited

Loading

codecov bot commented Mar 30, 2023 •

edited

Loading

Jorropo commented Mar 30, 2023 •

edited

Loading

hacdias left a comment •

edited

Loading

lidel left a comment •

edited

Loading