Skip to content

Remove AlignedArray and Aligned Matrix from src and tests #1028

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

Closed
wants to merge 7 commits into from
Closed

Remove AlignedArray and Aligned Matrix from src and tests #1028

wants to merge 7 commits into from

Conversation

Anipik
Copy link
Contributor

@Anipik Anipik commented Sep 25, 2018

@Anipik Anipik changed the title Remove AlignedArray and Aligned Matrix from src and tests [WIP]Remove AlignedArray and Aligned Matrix from src and tests Sep 25, 2018
@TomFinley
Copy link
Contributor

Sounds great to me, thanks @eerhardt .

@Anipik Anipik changed the title [WIP]Remove AlignedArray and Aligned Matrix from src and tests Remove AlignedArray and Aligned Matrix from src and tests Sep 28, 2018
@Anipik
Copy link
Contributor Author

Anipik commented Sep 29, 2018

@eerhardt @danmosemsft @TomFinley @tannergooding

Method without align align percentage slower
Test_Multiclass_WikiDetox_BigramsAndTrichar_OVAAveragedPerceptron 5.8 s 5.297 s 9.5
CV_Multiclass_WikiDetox_BigramsAndTrichar_OVAAveragedPerceptron 105 s 96.8 s 8
CV_Multiclass_WikiDetox_BigramsAndTrichar_LightGBMMulticlass 310 s 297 s 4
CV_Multiclass_WikiDetox_WordEmbeddings_OVAAveragedPerceptron 310 s 309 s 0
CV_Multiclass_WikiDetox_WordEmbeddings_SDCAMC 192 s 196 s -2%
TrainKMeansAndLR 242 ms 232 ms 4
MakeIrisPredictions 3 ms 3 ms 0
MakeSentimentPredictions 51 ms 53 ms -3
MakeBreastCancerPredictions 1.3 ms 1.3 ms 0
Test_Ranking_MSLRWeb10K_RawNumericFeatures_FastTreeRanking 1.4 s 1.1 s 3
TrainTest_Ranking_MSLRWeb10K_RawNumericFeatures_FastTreeRanking 33.91 s 32.7 s 3
TrainTest_Ranking_MSLRWeb10K_RawNumericFeatures_LightGBMRanking 34.33 s 32 s 7

updated the last row

@Anipik
Copy link
Contributor Author

Anipik commented Sep 30, 2018

The test failing is flaky.
I queued another build and it was successful. https://dnceng.visualstudio.com/public/_build/results?buildId=25891&view=results

@eerhardt
Copy link
Member

eerhardt commented Oct 1, 2018

I'm concerned with some of the perf degradation. We should investigate why these tests are that much slower.

@tannergooding
Copy link
Member

@eerhardt, I would suspect that many of the tests have small enough inputs that the cost of re-aligning was potentially small/negligible.

The current code also doesn't have the fixes that ensures most of the read/writes are actually aligned (except for the few leading/trailing unaligned elements, if any)

@tannergooding
Copy link
Member

The codegen is also going to be worse for the native intrinsic code, since it is emitting "legacy" encoded instructions (and won't be able to fold away the loads anymore).

I imagine the diff will be less for netcoreapp3.0 using the managed HWIntrinsics.

@eerhardt
Copy link
Member

eerhardt commented Oct 1, 2018

The current code also doesn't have the fixes that ensures most of the read/writes are actually aligned (except for the few leading/trailing unaligned elements, if any)

Can we prototype that and see if that fixes the perf regression?

@markusweimer
Copy link
Member

What is the status of this PR? I am asking because the last update has been some time ago, and it is now in conflict with master.

@Anipik
Copy link
Contributor Author

Anipik commented Oct 23, 2018

@markusweimer this pr is waiting on #1274 . i have the conflicts resolved in my local branch. probably #1274 will get merged by today

@Anipik
Copy link
Contributor Author

Anipik commented Oct 25, 2018

Closing this PR, will continue the work after my vacation

@Anipik Anipik closed this Oct 25, 2018
@TomFinley TomFinley mentioned this pull request Nov 8, 2018
@ghost ghost locked as resolved and limited conversation to collaborators Mar 28, 2022
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.

5 participants