WebRTC: don't use stack buffer
Created attachment 363933 [details] Patch
Attachment 363933 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 363941 [details] Patch
Attachment 363941 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 363947 [details] Patch
Attachment 363947 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 363957 [details] Patch
Attachment 363957 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 363957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363957&action=review > Source/WebCore/ChangeLog:3 > + WebRTC: don't create huge stack buffer in a loop Probably missing https://bugs.webkit.org/show_bug.cgi?id=195436 here. > Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.cpp:99 > + m_data.reset(new char[bytesPerSample * channels * samplesPerFrame]); Would better read with make_unique?
Created attachment 363958 [details] Patch Address comments.
Comment on attachment 363958 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363958&action=review > Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.cpp:99 > + m_data.reset(std::make_unique<char[]>(bytesPerSample * channels * samplesPerFrame)); Would use m_data = std::make_unique. If we go down that road, we could also allocate m_data in StartPlayoutOnAudioThread and delete it when exiting of StartPlayoutOnAudioThread. That would remove the if check and deallocate it when no longer needed.
Created attachment 363960 [details] Patch (In reply to youenn fablet from comment #11) > Comment on attachment 363958 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363958&action=review > > > Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.cpp:99 > > + m_data.reset(std::make_unique<char[]>(bytesPerSample * channels * samplesPerFrame)); > > Would use m_data = std::make_unique. Oops yes. Fixed. > If we go down that road, we could also allocate m_data in > StartPlayoutOnAudioThread and delete it when exiting of > StartPlayoutOnAudioThread. > That would remove the if check and deallocate it when no longer needed. I can do that if you want, I didn't dig through the code to convince myself that it was correct :-)
Comment on attachment 363960 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363960&action=review > Source/WebCore/ChangeLog:7 > + The 1920 byte buffer is pretty big, and the loop is unrolled 5 > + times in a regular build. If the unrolling is the issue, just putting the buffer outside the loop would solve the problem. Not sure allocating on the heap is the right thing.
Comment on attachment 363960 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363960&action=review >> Source/WebCore/ChangeLog:7 >> + times in a regular build. > > If the unrolling is the issue, just putting the buffer outside the loop would solve the problem. Not sure allocating on the heap is the right thing. My ulterior motive is that, once we automatically initialize this stack buffer, it'll be expensive. Without LTO the compiler won't be able to prove anything about the buffer being initialized somewhere else, so it'll call memset. There's a few other places in WebCore where this happens for somewhat large buffers.
(In reply to JF Bastien from comment #14) > My ulterior motive is that, once we automatically initialize this stack > buffer, it'll be expensive. OK.
(In reply to JF Bastien from comment #12) > Created attachment 363960 [details] > Patch > > (In reply to youenn fablet from comment #11) > > Comment on attachment 363958 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=363958&action=review > > > > > Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.cpp:99 > > > + m_data.reset(std::make_unique<char[]>(bytesPerSample * channels * samplesPerFrame)); > > > > Would use m_data = std::make_unique. > > Oops yes. Fixed. > > > > If we go down that road, we could also allocate m_data in > > StartPlayoutOnAudioThread and delete it when exiting of > > StartPlayoutOnAudioThread. > > That would remove the if check and deallocate it when no longer needed. > > I can do that if you want, I didn't dig through the code to convince myself > that it was correct :-) I think it is correct and would improve the code a bit. m_data could be replaced by a local var as well.
Your earlier version used operator new[] which I believe does not initialize the values to 0, and based on youenn's feedback you now use std::make_unique<char[]>, which I believe does initialize the values to 0. Is this true? Is this desired?
Comment on attachment 363960 [details] Patch This also doesn't use fastMalloc/fastFree, but makeUniqueArray does.