Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementing stable sort #809

Merged
merged 5 commits into from
Dec 10, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
40 changes: 40 additions & 0 deletions Jint.Tests/Runtime/ArrayTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -74,5 +74,45 @@ public void ArrayLengthFromInitialState()
var length = (int) array.Length;
Assert.Equal(0, length);
}

[Fact]
public void ArraySortIsStable()
{
const string code = @"
var items = [
{ name: 'Edward', value: 0 },
{ name: 'Sharpe', value: 0 },
{ name: 'And', value: 0 },
{ name: 'The', value: 1 },
{ name: 'Magnetic', value: 0 },
{ name: 'Zeros', value: 0 }
];

// sort by value
function compare(a, b) {
return a.value - b.value;
}

var a = items.sort();

assert(a[0].name == 'Edward');
assert(a[1].name == 'Sharpe');
assert(a[2].name == 'And');
assert(a[3].name == 'The');
assert(a[4].name == 'Magnetic');
assert(a[5].name == 'Zeros');

var a = items.sort(compare);

assert(a[0].name == 'Edward');
assert(a[1].name == 'Sharpe');
assert(a[2].name == 'And');
assert(a[3].name == 'Magnetic');
assert(a[4].name == 'Zeros');
assert(a[5].name == 'The');
";

_engine.Execute(code);
}
}
}
58 changes: 57 additions & 1 deletion Jint/Native/Array/ArrayOperations.cs
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
using System.Collections;
using System.Collections.Generic;
using Jint.Native.Number;
using Jint.Native.Object;
using Jint.Runtime;
using Jint.Runtime.Descriptors;

namespace Jint.Native.Array
{
internal abstract class ArrayOperations
internal abstract class ArrayOperations : IEnumerable<JsValue>
{
protected internal const ulong MaxArrayLength = 4294967295;
protected internal const ulong MaxArrayLikeLength = NumberConstructor.MaxSafeInteger;
Expand Down Expand Up @@ -68,6 +70,60 @@ public virtual JsValue[] GetAll(Types elementTypes)

public abstract void DeletePropertyOrThrow(ulong index);

public IEnumerator<JsValue> GetEnumerator()
Copy link
Collaborator

Choose a reason for hiding this comment

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

If there's an overload of concrete type public ArrayOperationsIterator GetEnumerator() then there would be no dispatch overhead like the interfaces have, CLR will find the version by name.

Copy link
Owner Author

Choose a reason for hiding this comment

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

What do you suggest? To implement it in each concrete type and call base.GetEnumerator() ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was referring to concrete enumerator method without interface, as seen here:

/// <summary>
/// Gets an enumerator over the dictionary
/// </summary>
public Enumerator GetEnumerator() => new Enumerator(this); // avoid boxing
/// <summary>
/// Gets an enumerator over the dictionary
/// </summary>
IEnumerator<KeyValuePair<TKey, TValue>> IEnumerable<KeyValuePair<TKey, TValue>>.GetEnumerator() =>
new Enumerator(this);
/// <summary>
/// Gets an enumerator over the dictionary
/// </summary>
IEnumerator IEnumerable.GetEnumerator() => new Enumerator(this);

Explicit interface implementations for generic use case and then one of concrete type to avoid boxing (struct via interface).

Copy link
Owner Author

Choose a reason for hiding this comment

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

Updated but this new member is not called, and we need to user OrderBy for a stable sort.

{
return new ArrayOperationsIterator(this);
}

IEnumerator IEnumerable.GetEnumerator()
{
return GetEnumerator();
}

private sealed class ArrayOperationsIterator : IEnumerator<JsValue>
sebastienros marked this conversation as resolved.
Show resolved Hide resolved
{
private readonly ArrayOperations _obj;
private ulong _current;
private bool _first;
lahma marked this conversation as resolved.
Show resolved Hide resolved
private readonly uint _length;

public ArrayOperationsIterator(ArrayOperations obj)
{
_obj = obj;
_length = obj.GetLength();

Reset();
}

public JsValue Current => _obj.TryGetValue(_current, out var temp) ? temp : null;

object IEnumerator.Current => Current;

public void Dispose()
{
}

public bool MoveNext()
{
if (_first)
{
_first = false;
}
else
{
_current++;
}

return _current < _length;
}

public void Reset()
{
_first = true;
_current = 0;
}
}

private sealed class ObjectInstanceOperations : ArrayOperations<ObjectInstance>
{
public ObjectInstanceOperations(ObjectInstance target) : base(target)
Expand Down
126 changes: 66 additions & 60 deletions Jint/Native/Array/ArrayPrototype.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Linq;
using Jint.Collections;
using Jint.Native.Object;
using Jint.Native.Symbol;
Expand Down Expand Up @@ -803,71 +804,14 @@ private JsValue Sort(JsValue thisObj, JsValue[] arguments)
return obj.Target;
}

int Comparer(JsValue x, JsValue y)
{
if (ReferenceEquals(x, null))
{
return 1;
}

if (ReferenceEquals(y, null))
{
return -1;
}

var xUndefined = x.IsUndefined();
var yUndefined = y.IsUndefined();
if (xUndefined && yUndefined)
{
return 0;
}

if (xUndefined)
{
return 1;
}

if (yUndefined)
{
return -1;
}

if (compareFn != null)
{
var s = TypeConverter.ToNumber(compareFn.Call(Undefined, new[] {x, y}));
if (s < 0)
{
return -1;
}

if (s > 0)
{
return 1;
}

return 0;
}

var xString = TypeConverter.ToString(x);
var yString = TypeConverter.ToString(y);

var r = CompareOrdinal(xString, yString);
return r;
}


var array = new JsValue[len];
for (uint i = 0; i < (uint) array.Length; ++i)
{
var value = obj.TryGetValue(i, out var temp)
? temp
: null;
array[i] = value;
}
JsValue[] array;

// don't eat inner exceptions
try
{
System.Array.Sort(array, Comparer);
array = obj.OrderBy(x => x, new ArrayComparer(compareFn)).ToArray();
sebastienros marked this conversation as resolved.
Show resolved Hide resolved
}
catch (InvalidOperationException e)
{
Expand Down Expand Up @@ -1290,5 +1234,67 @@ public JsValue Pop(JsValue thisObject, JsValue[] arguments)
o.SetLength(len);
return element;
}

private sealed class ArrayComparer : IComparer<JsValue>
sebastienros marked this conversation as resolved.
Show resolved Hide resolved
{
private readonly ICallable _compare;
public ArrayComparer(ICallable compare)
{
_compare = compare;
}

public int Compare(JsValue x, JsValue y)
{
if (ReferenceEquals(x, null))
{
return 1;
}

if (ReferenceEquals(y, null))
{
return -1;
}

var xUndefined = x.IsUndefined();
var yUndefined = y.IsUndefined();
if (xUndefined && yUndefined)
{
return 0;
}

if (xUndefined)
{
return 1;
}

if (yUndefined)
{
return -1;
}

if (_compare != null)
{
var s = TypeConverter.ToNumber(_compare.Call(Undefined, new[] {x, y}));
if (s < 0)
{
return -1;
}

if (s > 0)
{
return 1;
}

return 0;
}

var xString = TypeConverter.ToString(x);
var yString = TypeConverter.ToString(y);

var r = CompareOrdinal(xString, yString);
return r;
}
}
}

}