Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

List Clear+Remove#9540

Merged
jkotas merged 3 commits into
dotnet:masterfrom
benaadams:list-remove
Feb 12, 2017
Merged

List Clear+Remove#9540
jkotas merged 3 commits into
dotnet:masterfrom
benaadams:list-remove

Conversation

@benaadams
Copy link
Copy Markdown
Member

@benaadams benaadams commented Feb 12, 2017

List Clear is warmspot in Kestrel

Struct based Clear trims to

Marking List`1:Clear():this as NOINLINE because of unprofitable inline
Successfully inlined JitHelpers:ContainsReferences():bool (2 IL bytes) (depth 1) [below ALWAYS_INLINE size]
**************** Inline Tree
Inlines into 060034D8 List`1:Clear():this
  [1 IL=0000 TR=000001 06003B02] [below ALWAYS_INLINE size] JitHelpers:ContainsReferences():bool
  [0 IL=0029 TR=000037 06000248] [FAILED: noinline per IL/cached result] Array:Clear(ref,int,int)
Budget: initialTime=228, finalTime=218, initialBudget=2280, currentBudget=2280
Budget: initialSize=1408, finalSize=1408
; Assembly listing for method List`1:Clear():this
; Emitting BLENDED_CODE for X64 CPU with SSE2
; optimized code
; rsp based frame
; partially interruptible
; Final local variable assignments
;
;  V00 this         [V00,T00] (  7,   7  )     ref  ->  rcx         this
;  V01 OutArgs      [V01    ] (  1,   1  )  lclBlk (32) [rsp+0x00]  
;
; Lcl frame size = 40

G_M33881_IG01:
       4883EC28             sub      rsp, 40
       90                   nop      

G_M33881_IG02:
       33C0                 xor      eax, eax
       894118               mov      dword ptr [rcx+24], eax
       FF411C               inc      dword ptr [rcx+28]

G_M33881_IG03:
       4883C428             add      rsp, 40
       C3                   ret      

; Total bytes of code 18, prolog size 5 for method List`1:Clear():this

Shame about the no-inline...

For remove I was looking to swap Array.Copy for Buffer.Memmove or similar, however the Span code for getting a pointer to a T[] element in an array is a little exotic

if (_size > 0)
public void Clear()
{
if (JitHelpers.ContainsReferences<T>() && _size > 0)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other places in this file that call Array.Clear. We should fix them all.

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Feb 12, 2017

For remove I was looking to swap Array.Copy for Buffer.Memmove or similar, however the Span code for getting a pointer to a T[] element in an array is a little exotic

Right. Array.Copy is just fine for these (until we get optimized Span).

@benaadams
Copy link
Copy Markdown
Member Author

Tail calls into Array.Copy for reference based Clears

Marking List`1:Clear():this as NOINLINE because of unprofitable inline
Successfully inlined JitHelpers:ContainsReferences():bool (2 IL bytes) (depth 1) [below ALWAYS_INLINE size]
**************** Inline Tree
Inlines into 060034D8 List`1:Clear():this
  [1 IL=0000 TR=000001 06003B02] [below ALWAYS_INLINE size] JitHelpers:ContainsReferences():bool
  [0 IL=0047 TR=000087 06000248] [FAILED: noinline per IL/cached result] Array:Clear(ref,int,int)
Budget: initialTime=285, finalTime=275, initialBudget=2850, currentBudget=2850
Budget: initialSize=1841, finalSize=1841
; Assembly listing for method List`1:Clear():this
; Emitting BLENDED_CODE for X64 CPU with SSE2
; optimized code
; rsp based frame
; fully interruptible
; Final local variable assignments
;
;  V00 this         [V00,T00] ( 13,  12  )     ref  ->  rsi         this
;  V01 loc0         [V01,T01] (  5,   4  )     int  ->   r8        
;* V02 tmp0         [V02,T02] (  0,   0  )    long  ->  zero-ref   
;* V03 tmp1         [V03,T03] (  0,   0  )    long  ->  zero-ref   
;  V04 OutArgs      [V04    ] (  1,   1  )  lclBlk (32) [rsp+0x00]  
;
; Lcl frame size = 48

G_M58440_IG01:
       56                   push     rsi
       4883EC30             sub      rsp, 48
       48894C2428           mov      qword ptr [rsp+28H], rcx
       488BF1               mov      rsi, rcx

G_M58440_IG02:
       488B0E               mov      rcx, qword ptr [rsi]
       448B4618             mov      r8d, dword ptr [rsi+24]
       33C9                 xor      ecx, ecx
       894E18               mov      dword ptr [rsi+24], ecx
       FF461C               inc      dword ptr [rsi+28]
       4585C0               test     r8d, r8d
       7E15                 jle      SHORT G_M58440_IG04
       488B4E08             mov      rcx, gword ptr [rsi+8]
       33D2                 xor      edx, edx
       488D0500000000       lea      rax, [(reloc)]

G_M58440_IG03:
       4883C430             add      rsp, 48
       5E                   pop      rsi
       48FFE0               rex.jmp  rax

G_M58440_IG04:
       4883C430             add      rsp, 48
       5E                   pop      rsi
       C3                   ret      

; Total bytes of code 60, prolog size 13 for method List`1:Clear():this

@benaadams
Copy link
Copy Markdown
Member Author

benaadams commented Feb 12, 2017

@stephentoub @jkotas should arraypool clear anyway using this test when clearArray == false as it doesn't have a : struct constraint? i.e. to free the GC refs?

@stephentoub
Copy link
Copy Markdown
Member

should arraypool clear anyway using this test when clearArray == false as it doesn't have a : struct constraint? i.e. to free the GC refs?

You mean, if T is or contains refs, clear the array even if it wasn't requested, so as to avoid keeping objects alive? On the one hand, I can see the benefit in some cases; however I know there are cases (because I've done it) where code explicitly clears the used portion itself and then specifies clearArray==false, in which case the change would add cost for no benefit.

@benaadams
Copy link
Copy Markdown
Member Author

where code explicitly clears the used portion itself

Ah, makes sense

@benaadams
Copy link
Copy Markdown
Member Author

@jkotas added the other Array.Clears

The remaining ones in coreclr are ArrayPool; definite reference types (ArrayList); definite value types (Streams) so assume is conscious decision.

There is also Dictionary; but that has an array of Entrys and checks hashcode for zero; so would need to split to multi-arrays to benefit; but that has a high chance of being determental to perf elsewhere.

Looking for = deafult(T)

@benaadams
Copy link
Copy Markdown
Member Author

Remaining = deafult(T) clears are in ConcurrentQueue, ProducerConsumerQueues and ThreadLocal so I'll stay away from them 😄

@jkotas jkotas merged commit d400876 into dotnet:master Feb 12, 2017
@benaadams benaadams deleted the list-remove branch February 12, 2017 21:56
@omariom
Copy link
Copy Markdown

omariom commented Feb 12, 2017

👍 👍 👍

jorive pushed a commit to guhuro/coreclr that referenced this pull request May 4, 2017
* List Clear+Remove
* Tail call Array.Clear
@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* List Clear+Remove
* Tail call Array.Clear

Commit migrated from dotnet/coreclr@d400876
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants