8000 Dropping implementation of ArrayStorage · Issue #294 · SciSharp/NumSharp · GitHub
[go: up one dir, main page]
Skip to content

Dropping implementation of ArrayStorage #294

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.

Sign up for GitHub

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

Jump to bottom
Closed
Nucs opened this issue Jun 26, 2019 · 7 comments
Closed

Dropping implementation of ArrayStorage #294

Nucs opened this issue Jun 26, 2019 · 7 comments
Assignees
@henon @kerryjiang @Oceania2018 @deepakkumar1984
Labels
further discuss need further discuss to find the best solution

Comments

@Nucs
Copy link
Member
Nucs commented Jun 26, 2019 •
edited
Loading

After a deep look at the backend architecture and doing some experiments, I would like you to consider the following benchmark:

image

public double[] genericArray;
public Array nonGenericArray;

[GlobalSetup]
public void Setup()
{
    var rnd = new Random(42);
    // first array
    nonGenericArray = genericArray = new double[10_000_000];

    for (int i = 0; i < genericArray.Length; i++)
    {
        genericArray[i] = rnd.NextDouble();
    }
}

[Benchmark(Baseline = true)]
public void GenericAccess()
{
    var length = genericArray.Length;
    for (int i = 0; i < length; i++)
    {
        var a = genericArray[i];
    }
}

[Benchmark]
public void NonGenericAccess()
{
    double sum = 0;
    var length = nonGenericArray.Length;
    for (int i = 0; i < length; i++)
    {
        var a = nonGenericArray.GetValue(i);
    }
}

Proposition

I propose to drop-off non-generic ArrayStorage and only use TypedGenericStorage.

I have already done it in my local but I can't remove base classes without other architects's review.
@Oceania2018 Oceania2018 added the further discuss need further discuss to find the best solution label Jun 26, 2019
@Oceania2018
Copy link
Member

@Nucs Could you push GenericAccess into repo ? I can't find it.

image

Nucs added a commit that referenced this issue Jun 26, 2019
@Nucs
Added ArrayAccessing
6077862 
for issue #294
@henon
Copy link
Contributor
henon commented Jun 26, 2019 •
edited
Loading

My 2 cents:

  • if the benchmarks are in favor of your proposal (wow a factor 100 in some cases!!!)
  • all tests work with the new storage

Why the hell not ;)

This is a huge change though. Can you pull it off?

@Nucs
Copy link
Member Author
Nucs commented Jun 26, 2019 •
edited
Loading

@henon, Already did (thanks to regen), looking for approval.
I can't PR yet but I can't continue without the approval either.
And yes, existing tests already pass.
@Oceania2018 Added.

@Oceania2018
Copy link
Member
Oceania2018 commented Jun 26, 2019 •
edited
Loading

Just push to your repo. Github will put all commits in one PR automatically.

@henon
Copy link
Contributor
henon commented Jun 26, 2019

Awesome @Nucs, I am looking forward to the PR.

@Oceania2018
Copy link
Member

@Nucs Thank you for your nice comparison, actually, We're using the TypedArrayStorage not ArrayStorage, I've already found the big performance difference before.

I refactored it in TypedArrayStorage.

image

Our long term goal is implement another Storage -> ByteStorage to implement the way of zero-copy,
and share data with other libraries like ArrayFire, Apache Arrow.

@Nucs Nucs mentioned this issue Jun 28, 2019
Merged
@Nucs
Copy link
Member Author
Nucs commented Jul 1, 2019

Was dropped and successfully replaced with TypedArrayStorage.

@Nucs Nucs closed this as completed Jul 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@henon henon

@kerryjiang kerryjiang

@Oceania2018 Oceania2018

@deepakkumar1984 deepakkumar1984

Labels
further discuss need further discuss to find the best solution
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@henon @kerryjiang @Nucs @Oceania2018 @deepakkumar1984
0