sestoft / C5

C5 generic collection library for C#/.NET
http://www.itu.dk/research/c5/
MIT License
1.04k stars 179 forks source link

Unecessary allocations in HashSet<T> #138

Open jnyrup opened 1 year ago

jnyrup commented 1 year ago

Description

HashSet<T> contains three calls to string.Format in non-invariant checking methods. https://github.com/sestoft/C5/blob/21e90a22a4e75e6fe2d70d2c195b79830d5694ca/C5/Hashing/HashSet.cs#L127 https://github.com/sestoft/C5/blob/21e90a22a4e75e6fe2d70d2c195b79830d5694ca/C5/Hashing/HashSet.cs#L144 https://github.com/sestoft/C5/blob/21e90a22a4e75e6fe2d70d2c195b79830d5694ca/C5/Hashing/HashSet.cs#L167

Even though Logger.Log per default sends any string into void, this still results in some unnecessary string allocations.

Solution

I would suggest deleting those lines or hide them behind #if DEBUG

Describe alternatives you've considered

Additional context

sestoft commented 1 year ago

Hej Jonas

Er du interesseret i at bidrage direkte til vedligehold af C5?

De seneste år er det helt overvejende Rasmus Lystrøm (som formentlig auto-cc’s på denne mail) som har passet C5, da jeg er blevet institutleder og ikke kan holde trit med udviklingen i de 117.000 redskaber og frameworks der omgiver et stykke software i dag.

Peter

On 19 Jul 2023, at 15.52, Jonas Nyrup @.***> wrote:

Description

HashSet contains three calls to string.Format in non-invariant checking methods. https://github.com/sestoft/C5/blob/21e90a22a4e75e6fe2d70d2c195b79830d5694ca/C5/Hashing/HashSet.cs#L127 https://github.com/sestoft/C5/blob/21e90a22a4e75e6fe2d70d2c195b79830d5694ca/C5/Hashing/HashSet.cs#L144 https://github.com/sestoft/C5/blob/21e90a22a4e75e6fe2d70d2c195b79830d5694ca/C5/Hashing/HashSet.cs#L167

Even though Logger.Log per default sends any string into void, this still results in some unnecessary string allocations.

Solution

I would suggest deleting those lines or hide them behind #if DEBUG

Describe alternatives you've considered Additional context

— Reply to this email directly, view it on GitHubhttps://github.com/sestoft/C5/issues/138, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAFSQEEKDOGBPIV5MYJW7WLXQ7RDHANCNFSM6AAAAAA2P64DFM. You are receiving this because you are subscribed to this thread.Message ID: @.***>

jnyrup commented 1 year ago

Hej Peter

Det kunne jeg godt være interesseret i. For en god ordens skyld skal jeg lige have clearet det med min arbejdsgiver. De rette skal lige have afsluttet deres sommerferier, men jeg vender tilbage, når jeg har et svar.

jnyrup commented 1 year ago

Hej Peter og Rasmus

Så har jeg fået afklaring og jeg vil gerne hjælpe til med vedligeholdelsen af C5.

dlidstrom commented 1 year ago

Det ser ut som att dokumentationen har genererats felaktigt. Kanske du kan ta en titt på det också?

jnyrup commented 1 year ago

@dlidstrom opret venligst et separat GH issue til dette, det gør det meget lettere at holde styr på.

sestoft commented 1 year ago

Hej Jonas,

On 25 Aug 2023, at 12.03, Jonas Nyrup @.***> wrote:

Så har jeg fået afklaring og jeg vil gerne hjælpe til med vedligeholdelsen af C5.

Super.

Nu har Rasmus Lystrøm og jeg også langt om længe mødtes om det, og er enige om at det er meget fint hvis du bliver maintainer på C5.

Der er forskellige mere eller mindre praktiske forhold som skal ordnes (det har Rasmus mere forstand på).

Vi foreslår at mødes alle tre efter 23/10 - online (Teams eller Zoom) med mindre du tilfældigvis er i København alligevel - det står os ikke så klart om du til daglig er i Odense, Danmark, Seattle eller et helt andet sted. Vi er på Amager…

Peter

dlidstrom commented 3 weeks ago

Vad hände sen? Jag letar efter dokumentationen men hittar den inte längre.