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

Re: [PATCH] config: use a static lock_file struct

From
BWBrandon Williams <bmwill@google.com>
Date
Aug 30, 2017, 19:57 UTC
Message-ID
<20170830195731.GB50018@google.com>
In-Reply-To
<20170830195320.27w5mhnrcd2uosvz@sigill.intra.peff.net>
On 08/30, Jeff King wrote:
Show 26 quoted lines
> On Wed, Aug 30, 2017 at 08:55:01AM +0200, Michael Haggerty wrote:
> 
> > > +	tempfile->active = 0;
> > > +	for (p = &tempfile_list; *p; p = &(*p)->next) {
> > > +		if (*p == tempfile) {
> > > +			*p = tempfile->next;
> > > +			break;
> > > +		}
> > >  	}
> > >  }
> > 
> > `deactivate_tempfile()` is O(N) in the number of active tempfiles. This
> > could get noticeable for, say, updating 30k references, which involves
> > obtaining 30k reference locks. I think that code adds the locks in
> > alphabetical order and also removes them in alphabetical order, so the
> > overall effort would go like O(N²). I'm guessing that this would be
> > measurable but not fatal for realistic numbers of references, but it
> > should at least be benchmarked.
> > 
> > There are three obvious ways to make this O(1) again:
> > 
> > * Make the list doubly-linked.
> 
> Yeah, I noticed this when writing it, and the doubly-linked list was my
> first thought. It's much more straight-forward than making guesses about
> traversal order, and we already have a nice implementation in list.h.

I agree that a doubly-linked list is probably the best way to go in order to solve the performance issue.

Show 20 quoted lines
> 
> What made me hesitate for this demonstration was that it turns the
> removal into _two_ pointer updates. That made me more nervous about the
> racy case where we get a single handler while already deleting
> something. Specifically when we have "a <-> b <-> c" and we've updated
> "a->next" to point to "c" but "c->prev" still points to "b".
> 
> But even with the singly-linked list we're not fully race-proof
> (somebody could set *p to NULL between the time we look at it and
> dereference it). What we really care about is not two versions of the
> function running simultaneously, but one version getting interrupted by
> another and having the second one see a sane state (because we'll never
> return to the first signal handler; the second one will raise() and
> die).
> 
> And I think we're fine there even with a doubly-linked list. It's still
> the single update of the "next" pointer that controls that second
> traversal.
> 
> -Peff

I know it was mentioned earlier but if this is a critical section, and it would be bad if it was interrupted, then couldn't we turn off interrupts before attempting to remove an item from the list?

-- 
Brandon Williams
Previous: Jeff KingNext: Jeff King
Message 29 of 43 in “pkt-line: re-'static'-ify buffer in packet_write_fmt_1()”
  1. pkt-line: re-'static'-ify buffer in packet_write_fmt_1()Martin Ågren, Aug 27, 2017
  2. Jeff KingAug 27, 2017
  3. Jeff KingAug 27, 2017
  4. Lars SchneiderAug 27, 2017
  5. Martin ÅgrenAug 27, 2017
  6. Jeff KingAug 27, 2017
  7. Lars SchneiderAug 27, 2017
  8. Jeff KingAug 27, 2017
  9. Martin ÅgrenAug 28, 2017
  10. Stefan BellerAug 28, 2017
  11. Jeff KingAug 28, 2017
  12. Junio C HamanoSep 5, 2017
  13. Lars SchneiderAug 29, 2017
  14. Jeff KingAug 29, 2017
  15. config: use a static lock_file structJeff King, Aug 29, 2017
  16. Brandon WilliamsAug 29, 2017
  17. Brandon WilliamsAug 29, 2017
  18. Jeff KingAug 29, 2017
  19. Michael HaggertyAug 30, 2017
  20. Jeff KingAug 30, 2017
  21. Michael HaggertyAug 30, 2017
  22. Jeff KingAug 30, 2017
  23. Jeff KingAug 30, 2017
  24. Michael HaggertyAug 30, 2017
  25. Jeff KingSep 2, 2017
  26. Jeff KingSep 2, 2017
  27. Michael HaggertyAug 30, 2017
  28. Jeff KingAug 30, 2017
  29. Brandon WilliamsAug 30, 2017
  30. Jeff KingAug 30, 2017
  31. Brandon WilliamsAug 30, 2017
  32. Jeff KingAug 31, 2017
  33. Junio C HamanoSep 6, 2017
  34. Jeff KingSep 6, 2017
  35. Martin ÅgrenAug 29, 2017
  36. Jeff KingAug 29, 2017
  37. Jeff KingAug 30, 2017
  38. Junio C HamanoSep 5, 2017
  39. 0/2 Do not call cmd_*() as a subroutineJunio C Hamano, Oct 10, 2017
  40. 1/2 describe: do not use cmd_*() as a subroutineJunio C Hamano, Oct 10, 2017
  41. SZEDER GáborOct 10, 2017
  42. Junio C HamanoOct 11, 2017
  43. 2/2 merge-ours: do not use cmd_*() as a subroutineJunio C Hamano, Oct 10, 2017

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.