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

Re: [PATCH v2 08/11] reftable/writer: unify releasing memory

From
Han-Wen Nienhuys <hanwenn@gmail.com>
Date
Apr 4, 2024, 09:00 UTC
Message-ID
<CAOw_e7a9BnEh2OatwaGoSyVK46Wv2-sVkArbtXHLPt79b6g2qQ@mail.gmail.com>
In-Reply-To
<Zg5Xorz75NMRqONI@tanuki>
On Thu, Apr 4, 2024 at 9:32 AM Patrick Steinhardt <ps@pks.im> wrote:
Show 23 quoted lines
>
> On Thu, Apr 04, 2024 at 09:08:46AM +0200, Han-Wen Nienhuys wrote:
> > On Thu, Apr 4, 2024 at 7:48 AM Patrick Steinhardt <ps@pks.im> wrote:
> > > There are two code paths which release memory of the reftable writer:
> > >
> > >   - `reftable_writer_close()` releases internal state after it has
> > >     written data.
> > >
> > >   - `reftable_writer_free()` releases the block that was written to and
> > >     the writer itself.
> >
> > The bifurcation is there so you can read the stats after closing the
> > writer. The new method makes it harder to misuse, but now you have two
> > ways to end a writer. Suggestion: drop reftable_writer_{free,close}
> > from reftable-writer.h (rename to remove the reftable_ prefix because
> > they are no longer considered public) and find another way to read out
> > the stats. Either pass an optional reftable_writer_stats into the
> > construction of the writer, return the stats from the close function,
> > or drop stats altogether.  IIRC They are only used in the unit tests.
>
> But even with these refactorings the stats remain intact after calling
> `reftable_writer_close()` or `reftable_writer_release()`, right? So it
> basically continues to work as expected.
Right - I misinterpreted your change.
> It might not be the cleanest way to handle this, but I think this patch
> is an improvement over the previous state because we plug a memory leak
> and deduplicate the cleanup logic. So I would suggest to defer your
> proposed refactorings to a later point, if you're okay with that.

yes. Please add reftable_writer_release to reftable-writer.h for consistency, though. Or remove the reftable_ prefix.

-- 
Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen
Previous: Patrick SteinhardtNext: Patrick Steinhardt
Message 29 of 49 in “reftable: optimize write performance”
  1. 0/9 reftable: optimize write performancePatrick Steinhardt, Apr 2, 2024
  2. 1/9 refs/reftable: fix D/F conflict error message on ref copyPatrick Steinhardt, Apr 2, 2024
  3. Junio C HamanoApr 3, 2024
  4. 2/9 refs/reftable: perform explicit D/F check when writing symrefsPatrick Steinhardt, Apr 2, 2024
  5. 3/9 refs/reftable: skip duplicate name checksPatrick Steinhardt, Apr 2, 2024
  6. 4/9 refs/reftable: don't recompute committer identPatrick Steinhardt, Apr 2, 2024
  7. Junio C HamanoApr 3, 2024
  8. Patrick SteinhardtApr 4, 2024
  9. 5/9 reftable/writer: refactorings for `writer_add_record()`Patrick Steinhardt, Apr 2, 2024
  10. 6/9 reftable/writer: refactorings for `writer_flush_nonempty_block()`Patrick Steinhardt, Apr 2, 2024
  11. 7/9 reftable/block: reuse zstream when writing log blocksPatrick Steinhardt, Apr 2, 2024
  12. Junio C HamanoApr 3, 2024
  13. Patrick SteinhardtApr 4, 2024
  14. 8/9 reftable/block: reuse compressed arrayPatrick Steinhardt, Apr 2, 2024
  15. 9/9 reftable/writer: reset `last_key` instead of releasing itPatrick Steinhardt, Apr 2, 2024
  16. 00/11 reftable: optimize write performancePatrick Steinhardt, Apr 4, 2024
  17. 01/11 refs/reftable: fix D/F conflict error message on ref copyPatrick Steinhardt, Apr 4, 2024
  18. 02/11 refs/reftable: perform explicit D/F check when writing symrefsPatrick Steinhardt, Apr 4, 2024
  19. 03/11 refs/reftable: skip duplicate name checksPatrick Steinhardt, Apr 4, 2024
  20. 04/11 reftable: remove name checksPatrick Steinhardt, Apr 4, 2024
  21. 05/11 refs/reftable: don't recompute committer identPatrick Steinhardt, Apr 4, 2024
  22. 06/11 reftable/writer: refactorings for `writer_add_record()`Patrick Steinhardt, Apr 4, 2024
  23. Han-Wen NienhuysApr 4, 2024
  24. Patrick SteinhardtApr 4, 2024
  25. 07/11 reftable/writer: refactorings for `writer_flush_nonempty_block()`Patrick Steinhardt, Apr 4, 2024
  26. 08/11 reftable/writer: unify releasing memoryPatrick Steinhardt, Apr 4, 2024
  27. Han-Wen NienhuysApr 4, 2024
  28. Patrick SteinhardtApr 4, 2024
  29. Han-Wen NienhuysApr 4, 2024
  30. Patrick SteinhardtApr 4, 2024
  31. 09/11 reftable/writer: reset `last_key` instead of releasing itPatrick Steinhardt, Apr 4, 2024
  32. 10/11 reftable/block: reuse zstream when writing log blocksPatrick Steinhardt, Apr 4, 2024
  33. 11/11 reftable/block: reuse compressed arrayPatrick Steinhardt, Apr 4, 2024
  34. Han-Wen NienhuysApr 4, 2024
  35. Patrick SteinhardtApr 4, 2024
  36. 00/11 reftable: optimize write performancePatrick Steinhardt, Apr 8, 2024
  37. 01/11 refs/reftable: fix D/F conflict error message on ref copyPatrick Steinhardt, Apr 8, 2024
  38. 02/11 refs/reftable: perform explicit D/F check when writing symrefsPatrick Steinhardt, Apr 8, 2024
  39. 03/11 refs/reftable: skip duplicate name checksPatrick Steinhardt, Apr 8, 2024
  40. 04/11 reftable: remove name checksPatrick Steinhardt, Apr 8, 2024
  41. 05/11 refs/reftable: don't recompute committer identPatrick Steinhardt, Apr 8, 2024
  42. 06/11 reftable/writer: refactorings for `writer_add_record()`Patrick Steinhardt, Apr 8, 2024
  43. 07/11 reftable/writer: refactorings for `writer_flush_nonempty_block()`Patrick Steinhardt, Apr 8, 2024
  44. 08/11 reftable/writer: unify releasing memoryPatrick Steinhardt, Apr 8, 2024
  45. 09/11 reftable/writer: reset `last_key` instead of releasing itPatrick Steinhardt, Apr 8, 2024
  46. 10/11 reftable/block: reuse zstream when writing log blocksPatrick Steinhardt, Apr 8, 2024
  47. 11/11 reftable/block: reuse compressed arrayPatrick Steinhardt, Apr 8, 2024
  48. Junio C HamanoApr 9, 2024
  49. Patrick SteinhardtApr 9, 2024

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.