Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(129)

Issue 2509733003: Rewrite of sigslot that avoids vtables. (Closed)

Created:
4 years, 1 month ago by Taylor Brandstetter
Modified:
3 years, 9 months ago
Reviewers:
tommi, noahric
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Rewrite of sigslot that avoids vtables. This reduces binary size considerably and solves some other problems. Also rewrote using variadic templates. Initial patch contributed by andrey.semashev@gmail.com. BUG=webrtc:2305 Review-Url: https://codereview.webrtc.org/2509733003 Cr-Commit-Position: refs/heads/master@{#16703} Committed: https://chromium.googlesource.com/external/webrtc/+/8d517c41700dd03797bac630478e38b89c13a565

Patch Set 1 #

Patch Set 2 : Going back to tab formatting. #

Total comments: 1

Patch Set 3 : Fixing sctptransport compile error #

Patch Set 4 : Use static local variable instead of global, to make chromium happy. #

Patch Set 5 : Rewrite with variadic templates #

Patch Set 6 : Make "has_slots" virtual for backwards compatibility #

Patch Set 7 : Attempting to fix Windows issue due to different function pointer sizes. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+266 lines, -2442 lines) Patch
M webrtc/base/sigslot.h View 1 2 3 4 5 6 9 chunks +257 lines, -2397 lines 1 comment Download
M webrtc/base/sigslot.cc View 1 2 3 1 chunk +4 lines, -36 lines 0 comments Download
M webrtc/base/sigslot_unittest.cc View 1 2 1 chunk +2 lines, -6 lines 0 comments Download
M webrtc/media/sctp/sctptransport_unittest.cc View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M webrtc/p2p/base/stunserver.h View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 31 (18 generated)
Taylor Brandstetter
Peter, we should look at this some time. It reduced the binary size of libjingle_peerconnection.a ...
4 years, 1 month ago (2016-11-16 21:57:33 UTC) #2
Taylor Brandstetter
I went ahead and made a variadic template version. One problem I encountered is that ...
3 years, 11 months ago (2017-01-21 20:34:01 UTC) #7
Taylor Brandstetter
I ran some tests on the performance bots, but the results didn't show any statistical ...
3 years, 11 months ago (2017-01-22 07:47:37 UTC) #8
Taylor Brandstetter
Ping. By the way, I rewrote it using variadic templates, which reduces the amount of ...
3 years, 10 months ago (2017-02-13 23:56:11 UTC) #9
Taylor Brandstetter
Tommi: I know I'm throwing a bunch of things your way recently, but could you ...
3 years, 10 months ago (2017-02-14 04:27:19 UTC) #12
tommi
Thanks for doing this! The code was a little hard to review since I'm not ...
3 years, 10 months ago (2017-02-14 10:06:02 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2509733003/100001
3 years, 10 months ago (2017-02-14 10:06:17 UTC) #15
commit-bot: I haz the power
Try jobs failed on following builders: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/18626)
3 years, 10 months ago (2017-02-14 11:07:38 UTC) #17
tommi
Looks like all bots are good to go now - commit?
3 years, 10 months ago (2017-02-19 11:54:36 UTC) #22
Taylor Brandstetter
On 2017/02/19 11:54:36, tommi (webrtc) wrote: > Looks like all bots are good to go ...
3 years, 10 months ago (2017-02-19 18:30:42 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2509733003/120001
3 years, 10 months ago (2017-02-19 21:21:22 UTC) #26
commit-bot: I haz the power
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/8d517c41700dd03797bac630478e38b89c13a565
3 years, 10 months ago (2017-02-19 22:12:30 UTC) #29
noahric
3 years, 9 months ago (2017-03-14 03:15:53 UTC) #31
Message was sent while issue was closed.
https://codereview.webrtc.org/2509733003/diff/120001/webrtc/base/sigslot.h
File webrtc/base/sigslot.h (right):

https://codereview.webrtc.org/2509733003/diff/120001/webrtc/base/sigslot.h#ne...
webrtc/base/sigslot.h:416: connections_list::const_iterator it =
m_connected_slots.begin();
I think this broke sigslot copying. The old code used hs.m_senders, and this
just enumerates m_connected_slots instead of s.m_connected_slots (and then adds
the results back to m_connected_slots).

...which also means that none of the sigslot tests must test this :(

Powered by Google App Engine
This is Rietveld 408576698