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

Stack<T> optimization of (Try)Peek, (Try)Pop and Push #26086

Merged
merged 8 commits into from
Jan 30, 2018
Merged

Stack<T> optimization of (Try)Peek, (Try)Pop and Push #26086

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c124114
Stack.Pop RCE
gfoidl Dec 28, 2017
0123330
Applied same optimizations as for Pop on Peek, TryPeek, TryPop, Push
gfoidl Dec 28, 2017
e7308d8
Revert change for Push
gfoidl Dec 28, 2017
d2f5b2c
Stack.Push with hot-/cold-path (PushWithResize)
gfoidl Dec 28, 2017
f337882
Addressed PR feedback
gfoidl Dec 28, 2017
b0bfd83
Array-copy in Peek is not necessary, JIT can do the same
gfoidl Dec 29, 2017
0a2e11e
Reverted b0bfd83
gfoidl Jan 13, 2018
084acb7
Addressed PR feedback
gfoidl Jan 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 49 additions & 15 deletions src/System.Collections/src/System/Collections/Generic/Stack.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,69 +202,103 @@ public void TrimExcess()
// is empty, Peek throws an InvalidOperationException.
public T Peek()
{
if (_size == 0)
int size = _size - 1;
T[] array = _array;
Copy link

@mikedn mikedn Dec 29, 2017

Choose a reason for hiding this comment

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

Is this copy really necessary?

Copy link
Member Author

@gfoidl gfoidl Dec 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

With the copy T[] array = _array it gets:

000007fe`7bafc5d4 8b4110          mov     eax,dword ptr [rcx+10h]
000007fe`7bafc5d7 ffc8            dec     eax
000007fe`7bafc5d9 488b5108        mov     rdx,qword ptr [rcx+8]
000007fe`7bafc5dd 448b4208        mov     r8d,dword ptr [rdx+8]
000007fe`7bafc5e1 443bc0          cmp     r8d,eax
000007fe`7bafc5e4 760c            jbe     000007fe`7bafc5f2
000007fe`7bafc5e6 4863c0          movsxd  rax,eax
000007fe`7bafc5e9 8b448210        mov     eax,dword ptr [rdx+rax*4+10h]
000007fe`7bafc5ed 4883c428        add     rsp,28h
000007fe`7bafc5f1 c3              ret

Without the copy and

if ((uint)size >= (uint)_array.Length)
...
return _array[size];

it gets:

000007fe`7bb0c5d4 8b4110          mov     eax,dword ptr [rcx+10h]
000007fe`7bb0c5d7 ffc8            dec     eax
000007fe`7bb0c5d9 488b5108        mov     rdx,qword ptr [rcx+8]
000007fe`7bb0c5dd 448b4208        mov     r8d,dword ptr [rdx+8]
000007fe`7bb0c5e1 443bc0          cmp     r8d,eax
000007fe`7bb0c5e4 760c            jbe     000007fe`7bb0c5f2
000007fe`7bb0c5e6 4863c0          movsxd  rax,eax
000007fe`7bb0c5e9 8b448210        mov     eax,dword ptr [rdx+rax*4+10h]
000007fe`7bb0c5ed 4883c428        add     rsp,28h
000007fe`7bb0c5f1 c3              ret

So basically the same code. Hence the copy can be avoided.

It seems as the JIT elides bound checks even on field access. Is this new? (and is this safe?)

Copy link

@mikedn mikedn Dec 29, 2017

Choose a reason for hiding this comment

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

It seems as the JIT elides bound checks even on field access. Is this new? (and is this safe?)

It's safe in the sense that the JIT generates correct code. Note that there's only one field load in the generated code. CSE removed the second one so in effect it did the same optimization that you did manually.

The fact that the range check is actually removed might be relatively new, in the past the JIT did not propagate existing analysis information to variables created by CSE and this usually prevented further optimizations. Some improvements in this area were done in dotnet/coreclr#9615

In general I'd recommend avoiding multiple field loads when arrays are involved, CSE may not be able to always eliminate redundant loads and even if it does there may still be cases where it doesn't work as well as manual optimization.

Still, in trivial cases such as Peek it should work fine and thus there's little reason to complicate the code.

Copy link
Member Author

@gfoidl gfoidl Dec 29, 2017

Choose a reason for hiding this comment

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

Thanks for the great reply. 👍

I'll update the PR for this change.

Copy link
Member Author

@gfoidl gfoidl Dec 29, 2017

Choose a reason for hiding this comment

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

For reference in TryPeek this JIT-optimization doesn't apply:

With Copy

000007fe`7b86c610 8b4110          mov     eax,dword ptr [rcx+10h]
000007fe`7b86c613 448d40ff        lea     r8d,[rax-1]
000007fe`7b86c617 488b4908        mov     rcx,qword ptr [rcx+8]
000007fe`7b86c61b 44394108        cmp     dword ptr [rcx+8],r8d
000007fe`7b86c61f 7705            ja      000007fe`7b86c626
000007fe`7b86c621 33c0            xor     eax,eax
000007fe`7b86c623 8902            mov     dword ptr [rdx],eax
000007fe`7b86c625 c3              ret
000007fe`7b86c626 4963c0          movsxd  rax,r8d
000007fe`7b86c629 8b448110        mov     eax,dword ptr [rcx+rax*4+10h]
000007fe`7b86c62d 8902            mov     dword ptr [rdx],eax
000007fe`7b86c62f b801000000      mov     eax,1

Without Copy

000007fe`7b87c610 8b4110          mov     eax,dword ptr [rcx+10h]
000007fe`7b87c613 448d40ff        lea     r8d,[rax-1]
000007fe`7b87c617 488b4108        mov     rax,qword ptr [rcx+8]
000007fe`7b87c61b 8b4008          mov     eax,dword ptr [rax+8]
000007fe`7b87c61e 413bc0          cmp     eax,r8d
000007fe`7b87c621 7705            ja      000007fe`7b87c628
000007fe`7b87c623 33c0            xor     eax,eax
000007fe`7b87c625 8902            mov     dword ptr [rdx],eax
000007fe`7b87c627 c3              ret
000007fe`7b87c628 488b4108        mov     rax,qword ptr [rcx+8]
000007fe`7b87c62c 4963c8          movsxd  rcx,r8d
000007fe`7b87c62f 8b448810        mov     eax,dword ptr [rax+rcx*4+10h]
000007fe`7b87c633 8902            mov     dword ptr [rdx],eax
000007fe`7b87c635 b801000000      mov     eax,1

Copy link

@mikedn mikedn Dec 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, interesting. The range check seems to have been eliminated but there are 2 loads of the _array field. That doesn't seem right, I'll have to look into this.

Copy link

@mikedn mikedn Dec 29, 2017

Choose a reason for hiding this comment

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

I'm not yet sure what's going on but the generated code is definitely incorrect. Created https://github.com/dotnet/coreclr/issues/15671


if ((uint)size >= (uint)array.Length)
{
ThrowForEmptyStack();
}

return _array[_size - 1];
return array[size];
}

public bool TryPeek(out T result)
{
if (_size == 0)
int size = _size - 1;
T[] array = _array;

if ((uint)size >= (uint)array.Length)
{
result = default(T);
result = default;
return false;
}
result = _array[_size - 1];
result = array[size];
return true;
}

// Pops an item from the top of the stack. If the stack is empty, Pop
// throws an InvalidOperationException.
public T Pop()
{
if (_size == 0)
int size = _size - 1;
T[] array = _array;

// if (_size == 0) is equivalent to if (size == -1), and this case
// is covered with (uint)size, thus allowing bounds check elimination
// https://github.com/dotnet/coreclr/pull/9773
if ((uint)size >= (uint)array.Length)
{
ThrowForEmptyStack();
}

_version++;
T item = _array[--_size];
_size = size;
T item = array[size];
if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
{
_array[_size] = default(T); // Free memory quicker.
array[size] = default; // Free memory quicker.
}
return item;
}

public bool TryPop(out T result)
{
if (_size == 0)
int size = _size - 1;
T[] array = _array;

if ((uint)size >= (uint)array.Length)
{
result = default(T);
result = default;
return false;
}

_version++;
result = _array[--_size];
_size = size;
result = array[size];
if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
{
_array[_size] = default(T); // Free memory quicker.
array[size] = default; // Free memory quicker.
}
return true;
}

// Pushes an item to the top of the stack.
public void Push(T item)
{
if (_size == _array.Length)
int size = _size;
T[] array = _array;

if ((uint)size < (uint)array.Length)
{
Array.Resize(ref _array, (_array.Length == 0) ? DefaultCapacity : 2 * _array.Length);
array[size] = item;
_version++;
_size = size + 1;
}
_array[_size++] = item;
else
{
PushWithResize(item);
}
}

// Non-inline from Stack.Push to improve its code quality as uncommon path
[MethodImpl(MethodImplOptions.NoInlining)]
private void PushWithResize(T item)
{
Array.Resize(ref _array, (_array.Length == 0) ? DefaultCapacity : 2 * _array.Length);
_array[_size] = item;
_version++;
_size++;
}

// Copies the Stack to an array, in the same order Pop would return the items.
Expand Down