Opened 8 months ago

Closed 7 months ago

#23953 closed enhancement (implemented)

Use stdatomic counters where available

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: review-group-25, review-group-26
Cc: Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

C11 has built-in atomics; we may as well use them to implement atomic_counter_t and speed up a little.

Child Tickets

Change History (9)

comment:1 Changed 8 months ago by nickm

Status: assignedneeds_review

See branch ticket23953_033

comment:2 Changed 7 months ago by nickm

Keywords: review-group-25 added

comment:3 Changed 7 months ago by dgoulet

Reviewer: dgoulet

comment:4 Changed 7 months ago by dgoulet

Status: needs_reviewneeds_revision
  • The destroy function, can't we just NOP it instead of doing this function call that I think the compiler will never optimize out because of this (void) thing?
+void
+atomic_counter_destroy(atomic_counter_t *counter)
+{
+  (void)counter;
+}

Into something like:

#define atomic_counter_destroy(c)

Second thing, in terms of performance, shouldn't those be better as static inline? I'm not sure how much they are used in Tor but if it is a _lot_, that could help a bit instead of a function jump.

One last tiny thing, if you could put the comment in the #else and #endif, it would be grand!

#ifdef HAVE_STDATOMIC_H
...
#else /* HAVE_STDATOMIC_H */
...
#endif /* HAVE_STDATOMIC_H */

comment:5 Changed 7 months ago by nickm

Status: needs_revisionneeds_review

I've added a commit for the inlining changes -- but I think the (void) won't actually stop the compiler from optimizing stuff -- it just tells it not to warn us about "counter" being unused.

For the annotations, I'd rather just run the `scripts/maint/annotate_ifdef_directives script from time to time, and let that make the changes for me?

comment:6 Changed 7 months ago by nickm

Keywords: review-group-26 added

Creating review-group-26.

comment:7 Changed 7 months ago by dgoulet

Status: needs_reviewmerge_ready

lgtm;

Fine with the annotate script also.

comment:8 Changed 7 months ago by nickm

thanks! Merged to master, and ran the script.

comment:9 Changed 7 months ago by nickm

Resolution: implemented
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.