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

Memory optimizations for all resolutions #242

Merged
merged 42 commits into from May 12, 2019

Conversation

Projects
None yet
8 participants
@hassount
Copy link
Contributor

commented May 9, 2019

35 - 70% memory savings across all presets / resolutions / core count.
Gain is highest for the smaller resolutions fastest preset.

@EwoutH

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

Using /usr/bin/time -v to measure the maximum resident set size, I noticed a decrease of 50% to 66% on -enc-mode 8 with 1080p 4:2:0 content.

for i in 48 32 24 16 12 8 6 4; do /usr/bin/time -v /home/ewouth/SVT-AV1/Bin/Release/SvtAv1EncApp -i Morocco-1000.y4m -lp $i -n 240 -enc-mode 8; done;
Memory (kB) Master #242
CPUs ad4522f 72c158a
4 5891848 2023840 -65,7%
6 6193776 2326164 -62,4%
8 6291768 2422412 -61,5%
12 6488312 2613680 -59,7%
16 9410104 3652892 -61,2%
24 10006232 4242508 -57,6%
32 10599800 4831604 -54,4%
48 13454776 6661304 -50,5%

And 32% to 40% on -enc mode 4.

Memory (kB) Master #242
CPUs ad4522f 72c158a
8 7938900 4614648 -41,9%
12 8127564 4835296 -40,5%
16 11157896 6380056 -42,8%
24 12229752 7383004 -39,6%
32 13237408 8309064 -37,2%
48 17458972 11824168 -32,3%

Amazing improvement!

@joelsole
Copy link
Contributor

left a comment

This hangs for Mac at the end in function eb_deinit_encoder - it doesn't exit the do-while loop

@kodabb

This comment has been minimized.

Copy link

commented May 9, 2019

this PR is un-reviewable -- don't get me wrong, I appreciate the improvements, but this code dump is full with commits that mean nothing or needlessly taint the history and it's devoid of any good development practice or open source community

@kylophone

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2019

this PR is un-reviewable -- don't get me wrong, I appreciate the improvements, but this code dump is full with commits that mean nothing or needlessly taint the history and it's devoid of any good development practice or open source community

I've also been mentioning this: diffs should contain only changes to the topic at hand. Mixing in unrelated changes and style cleanups only make things more unclear. It might be tempting to do so, but it should be avoided.

@joelsole
Copy link
Contributor

left a comment

It hangs here at
return_error = sem_close(semaphore_handle);
when
(EbHandle) $5 = 0x000000000000000b

Process 56491 stopped

  • thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
    frame #0: 0x000000010021eae3 libSvtAv1Enc.dylib`eb_destroy_semaphore(semaphore_handle=0x000000000000000b) at EbThreads.c:275
    272 return_error = sem_destroy((sem_t*)semaphore_handle) ? EB_ErrorDestroySemaphoreFailed : EB_ErrorNone;
    273 free(semaphore_handle);
    274 #elif defined(APPLE)
    -> 275 return_error = sem_close(semaphore_handle);
    276 #endif // _WIN32
    277
    278 return return_error;
Update MEM_MAP_OPT
+ address mac os deadlock: revert the order of the memory map linked list to start by stopping threads then destroy semaphores
@joelsole
Copy link
Contributor

left a comment

Mac issue seems solved. Thanks.

@hassount

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

@kodabb
Thank you for your feedback. Currently, as we are still in early development there are many major / complex features being added requiring multiple code changes. That said, we will work on improving our contribution practices and try to limit the size of patches. In the mean time, if you wish to review the changes apart from the small code cleanups, please fin the pre-compile macros under EbDefinitions.h that will guide you through the changes made in this PR.

@kylophone
We will try to restrain from including coding style cleanups into PRs. Although from the previous major PRs that we made to fix this, it made code merges quite challenging and this is why we thought adding a few here and there would help getting this done in an easier way that would not impact the speed of development.

@hassount hassount merged commit 430c2a6 into OpenVisualCloud:master May 12, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kodabb

This comment has been minimized.

Copy link

commented May 13, 2019

Currently, as we are still in early development there are many major / complex features being added requiring multiple code changes

That is really a poor excuse, especially since you're at the beginning of a project you should try and keep your git history as clean as possible: fewer lines of codes and fewer git commits. There is very little value in commit hygiene after the history is tainted.

try to limit the size of patches

The size of a patch is not the problem here: it's the size of the PR which mixes different changes and contains commits that change and change back code subsequently. Both of these practices should be avoided, and proper --amend and --rebase commands should be used instead to make the PR presentable, and understandable by developers from outside this project.

that would not impact the speed of development.

I keep hearing this argument when i bring up development quality, but it can't be further from the truth. Once proper practices are in place, it takes very little time to apply them and the pay off is in shorter debug runs (as you can check history faster and more cleaner) and better understanding of the codebase itself (as commit logs should include a bit of explanation that you cannot find documented anywhere else).

The only good the current practice's doing is only acting as a gatekeeper for people who actually care about those points, a.k.a. any open source multimedia developer, who, according to various marketing announcements, you are seeking to entail in this project. I am sure that your goal is not to make another Android-style code dump repository, and I am hopeful that you will improve your code quality before your codec quality.

@jbkempf

This comment has been minimized.

Copy link

commented May 13, 2019

I can only support @kodabb here. Those kind of PR are ridiculous.
I won't even comment about the code (who thought that adding defines was a good idea???), but the commit logs is a total mess: either massive MR or 42 patches that do not compile one after each other (some commits have a few lines only).
And the debate about the speed of development is absolutely ridiculous: "I commit crap because I need to code fast". First, you don't, and improving code quality and logs is essential for quality, if you want to keep it up and not finish like libaom.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.