git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFC PATCH 1/6] bloom: annotate filters with hash version

From
Taylor Blau <me@ttaylorr.com>
Date
Aug 17, 2023, 19:55 UTC
Message-ID
<ZN57Gsz+wk9n6/Da@nand.local>
In-Reply-To
<20230811214651.3326180-1-jonathantanmy@google.com>
On Fri, Aug 11, 2023 at 02:46:51PM -0700, Jonathan Tan wrote:
Show 9 quoted lines
> Taylor Blau <me@ttaylorr.com> writes:
> > In subsequent commits, we will want to load existing Bloom filters out
> > of a commit-graph, even when the hash version they were computed with
> > does not match the value of `commitGraph.changedPathVersion`.
> >
> > In order to differentiate between the two, add a "filter" field to each
> > Bloom filter.
>
> You mean "version", I think.

Oops, yes -- I'm not sure how my editor tab-completed "version" there, but oh, well :-).

Show 11 quoted lines
> > @@ -55,6 +55,7 @@ struct bloom_filter_settings {
> >  struct bloom_filter {
> >  	unsigned char *data;
> >  	size_t len;
> > +	int version;
> >  };
>
> We might want to shrink the sizes of len (we have a changed path limit
> so we know exactly how big Bloom filters can get) and version so that
> this struct doesn't take up more space. But if other reviewers think
> that this is OK, I'm fine with that.

I think that making len a size_t here is an appropriate choice. Even though the maximum length of a Bloom filter is well below the 2^64-1 threshold, we are often looking at a memory-mapped region here, so keeping track of it with a size_t / off_t seems reasonable to me.

Show 7 quoted lines
> Another thing that we might want to track is whether the Bloom filter is
> a reference to an existing buffer (and thus does not need to be freed)
> or a reference to a malloc-ed buffer that we must free. But both before
> and after this patch set, a malloc-ed buffer is never overridden by a
> reference-to-existing-buffer, so we should still be fine for now. (This
> patch set does add a scenario in which a reference-to-existing buffer is
> overridden by a malloc-ed buffer, but that's the only new scenario.)

Yeah, I think there is some opportunity for clean-up here. I'll take a look...

Thanks, Taylor

Previous: Jonathan TanNext: Taylor Blau
Message 19 of 24 in “bloom: reuse existing Bloom filters when possible during upgrade”
  1. 0/6 bloom: reuse existing Bloom filters when possible during upgradeTaylor Blau, Aug 7, 2023
  2. 3/6 t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`Taylor Blau, Aug 7, 2023
  3. 4/6 commit-graph.c: unconditionally load Bloom filtersTaylor Blau, Aug 7, 2023
  4. Jonathan TanAug 11, 2023
  5. Taylor BlauAug 21, 2023
  6. 2/6 bloom: prepare to discard incompatible Bloom filtersTaylor Blau, Aug 7, 2023
  7. Jonathan TanAug 11, 2023
  8. Taylor BlauAug 21, 2023
  9. Jonathan TanAug 24, 2023
  10. Taylor BlauAug 24, 2023
  11. Jonathan TanAug 24, 2023
  12. Taylor BlauAug 25, 2023
  13. Jonathan TanAug 29, 2023
  14. Taylor BlauAug 29, 2023
  15. Jonathan TanAug 29, 2023
  16. 5/6 object.h: fix mis-aligned flag bits tableTaylor Blau, Aug 7, 2023
  17. 1/6 bloom: annotate filters with hash versionTaylor Blau, Aug 7, 2023
  18. Jonathan TanAug 11, 2023
  19. Taylor BlauAug 17, 2023
  20. Taylor BlauAug 21, 2023
  21. 6/6 commit-graph: reuse existing Bloom filters where possibleTaylor Blau, Aug 7, 2023
  22. Jonathan TanAug 11, 2023
  23. Jonathan TanAug 11, 2023
  24. Taylor BlauAug 21, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.