| 02:00.40 | *** join/#asterisk-dev protocoldoug (~quassel@unaffiliated/protocoldoug) |
| 02:19.11 | *** join/#asterisk-dev coreyf1513 (~cfarrell@ws2.cfware.com) |
| 02:24.54 | *** join/#asterisk-dev mjordan (~matt@75.76.55.191) |
| 02:24.55 | *** mode/#asterisk-dev [+o mjordan] by ChanServ |
| 02:41.04 | coreyf1513 | mjordan: on the ref_log review I think you missed my comment about AO2_GLOBAL_OBJ_STATIC |
| 02:42.08 | mjordan | coreyf1513: hm... which review was that one? I didn't see it in the latest round |
| 02:42.30 | mjordan | ah, found it |
| 02:42.58 | coreyf1513 | yeah.. that's the example I know of where ao2 can be unref'ed by __attribute__((destructor)) |
| 02:47.16 | *** join/#asterisk-dev mjordan (~matt@75.76.55.191) |
| 02:47.17 | *** mode/#asterisk-dev [+o mjordan] by ChanServ |
| 02:47.25 | mjordan | coreyf1513: sorry, laptop crapped out |
| 02:47.28 | mjordan | so, that's still not a problem. |
| 02:47.38 | mjordan | if you use a global object container in a module, there's two things that will happen |
| 02:48.01 | mjordan | (1) in the module unload, the object in the container should be destroyed. That's the "right" thing to do. That means nothing gets de-ref'd in the destructor |
| 02:48.23 | mjordan | (2) if they don't do things correctly, then the destructor is called in dlclose, which happens before the cleanup function |
| 02:48.40 | coreyf1513 | hmm let me grep something real quick |
| 02:48.40 | mjordan | now, there is a problem if a global container is used in the core (asterisk) |
| 02:48.49 | coreyf1513 | that's what I was about to look for :) |
| 02:48.51 | mjordan | the destructor there will be used when main exits, which happens after the astobj2 init |
| 02:48.57 | mjordan | however, we should generally still be okay |
| 02:49.20 | coreyf1513 | makes me feel like AO2_GLOBAL_OBJ_STATIC should just let the ref leak if proper shutdown isn't done |
| 02:49.21 | mjordan | (1) the thing in the container should be cleaned up (and thus gone) by the time the astobj2 cleanup function is called, as each core module should clean up its global objects |
| 02:49.38 | mjordan | I'd be fine with that. I think Richard probably added that as an 'oh crap' |
| 02:49.47 | mjordan | I'd rather ask him tomorrow however, as he is crafty about such things |
| 02:49.59 | mjordan | or (2), even if it is still in there, it will just hit a ref_log that is now NULL |
| 02:50.09 | mjordan | as this is all in the core, and there is only one thread unloading everything |
| 02:50.16 | coreyf1513 | sure. so the other thing with shutdown is non-graceful shutdown |
| 02:50.52 | mjordan | in a non-graceful shutdown, things typically leak |
| 02:51.01 | mjordan | and even in that case, there's still a single thread that runs the cleanups |
| 02:51.33 | coreyf1513 | thats fine I'm not worried about that, i'm worry about the race between astobj2_cleanup (an ast_register_atexit procedure), and threads that have not stopped |
| 02:52.16 | coreyf1513 | IE what happens with fprintf(ref_log, "...."); when ref_log == NULL? same question if ref_log != NULL but has been fclose'd. |
| 02:53.00 | mjordan | looks at something |
| 02:55.09 | mjordan | it isn't impossible. At the same time, pretty unlikely and if you're hitting core stop now, you're already going down in flames. REF_DEBUG is already something that would be enabled only if you're hunting down a problem; I'm not sure it's really going to be a problem in practice. |
| 02:55.28 | coreyf1513 | fair enough |
| 02:55.31 | mjordan | I will say this: opening and closing the file every time suuuucks |
| 02:55.45 | mjordan | running Asterisk with a large refs file gets prettty slow |
| 02:55.57 | mjordan | I'm hoping that I don't have to add that in again :-) |
| 02:56.34 | coreyf1513 | agreed. so when you move ast_register_atexit(astobj2_cleanup); out of the #ifdef, maybe you can switch it to ast_register_cleanup in 11+? |
| 02:56.38 | mjordan | coreyf1513: thanks for giving this a thorough review however. You're one of the reasons why I wanted to fix this up, given how much cleanup work you've been doing. |
| 02:56.44 | mjordan | coreyf1513: yeah, that's what I'm thinking |
| 02:56.48 | mjordan | did you backport cleanup to 1.8? |
| 02:57.01 | coreyf1513 | no :( makes me wish the format.c bug applied to 1.8 |
| 02:57.06 | mjordan | heh |
| 02:57.18 | mjordan | well, 11 I'll switch the ref log close to cleanup only |
| 02:57.55 | mjordan | given the prevalence of ao2 in 11+ (and even more so in 12), we're more likely to hit funky things like that in those versions over 1.8 anyway |
| 02:59.59 | coreyf1513 | oh. so about including utils/refcounter or not.. you still think the old utility should not be removed from 1.8/11/12? |
| 03:23.29 | mjordan | ah |
| 03:26.18 | mjordan | coreyf1513: so, sometimes when a file is removed from the source tree, people who update can run into issues with the file lingering |
| 03:26.43 | mjordan | that was rmudgett's point - "Otherwise, you could have a stale refcounter program lying around after an svn up." |
| 03:26.52 | mjordan | in this case, it wouldn't hurt much, since almost no one compiles utils |
| 03:27.06 | mjordan | but if they had left compilation of utils on, they could end up in some funkiness. |
| 03:29.57 | coreyf1513 | you mean if you delete utils/refcounter.c and someone was compiling it, that would cause their build to fail? |
| 03:34.18 | coreyf1513 | mjordan: if that's the case can we reduce utils/refcounter.c in existing branches to mxml tags plus: int main() { printf("This utility has been replaced, the new script is in the Asterisk source, contrib/scripts/refcounter.py\n"); } |
| 03:34.38 | mjordan | coreyf1513: that works for me. I'll push a review up for that tomorrow |
| 03:34.47 | mjordan | I've been thinking some more about core stop now. I may throw in a lock. |
| 03:34.57 | mjordan | it wouldn't be a lot of code. |
| 03:35.06 | coreyf1513 | a lock for every write? |
| 03:35.16 | mjordan | pretty much. Only way to ensure atomicity. |
| 03:35.31 | coreyf1513 | ewww.... |
| 03:35.34 | mjordan | yup! |
| 03:35.37 | mjordan | unloading sucks :-) |
| 03:35.44 | coreyf1513 | so that's for 1.8 only then? |
| 03:36.06 | mjordan | probably. Or we could just not care about 1.8. |
| 03:36.13 | mjordan | I'll take a look at it at least and think about it some more. |
| 03:36.30 | coreyf1513 | isn't there a master refcount for ao2? |
| 03:36.52 | coreyf1513 | maybe only close if the master count is already zero? |
| 03:37.46 | mjordan | that would mean that any leaked object would result in the file not getting closed |
| 03:38.33 | coreyf1513 | I meant as a workaround for 1.8.. or we could just finish the backport of ast_register_cleanup :) |
| 05:21.59 | *** join/#asterisk-dev snuff-work (~snuffy@1.144.189.209) |
| 05:21.59 | *** mode/#asterisk-dev [+o snuff-work] by ChanServ |
| 06:19.23 | *** join/#asterisk-dev oej (~Adium@h87-96-134-129.dynamic.se.alltele.net) |
| 06:19.23 | *** mode/#asterisk-dev [+o oej] by ChanServ |
| 07:43.21 | *** join/#asterisk-dev oej (~Adium@h87-96-134-129.dynamic.se.alltele.net) |
| 07:43.22 | *** mode/#asterisk-dev [+o oej] by ChanServ |
| 08:11.54 | *** join/#asterisk-dev danjenkins (~dan@cpc65687-folk2-2-0-cust207.1-2.cable.virginmedia.com) |
| 08:39.05 | *** join/#asterisk-dev danjenkins (~dan@cpc65687-folk2-2-0-cust207.1-2.cable.virginmedia.com) |
| 09:07.51 | skrusty | morning |
| 09:35.32 | *** join/#asterisk-dev danjenkins (~dan@cpc65687-folk2-2-0-cust207.1-2.cable.virginmedia.com) |
| 10:35.43 | file | g'morning |
| 10:49.49 | *** join/#asterisk-dev danjenkins_ (~dan@cpc7-folk2-2-0-cust123.1-2.cable.virginm.net) |
| 10:52.56 | *** join/#asterisk-dev vlad_starkov (~vlad_star@91.206.59.99) |
| 11:15.05 | *** join/#asterisk-dev danjenkins (~dan@cpc65687-folk2-2-0-cust207.1-2.cable.virginmedia.com) |
| 11:42.12 | *** join/#asterisk-dev tzafrir (~tzafrir@bzq-218-28-58.cablep.bezeqint.net) |
| 11:42.13 | *** mode/#asterisk-dev [+o tzafrir] by ChanServ |
| 11:43.49 | *** join/#asterisk-dev danjenkins_ (~dan@cpc65687-folk2-2-0-cust207.1-2.cable.virginmedia.com) |
| 11:50.04 | *** join/#asterisk-dev danjenkins (~dan@cpc65687-folk2-2-0-cust207.1-2.cable.virginmedia.com) |
| 12:01.30 | *** join/#asterisk-dev pc-m (~pcm@modemcable094.94-70-69.static.videotron.ca) |
| 12:11.57 | *** join/#asterisk-dev oej (~Adium@h87-96-134-129.dynamic.se.alltele.net) |
| 12:11.57 | *** mode/#asterisk-dev [+o oej] by ChanServ |
| 12:11.57 | *** join/#asterisk-dev danjenkins (~dan@cpc65687-folk2-2-0-cust207.1-2.cable.virginmedia.com) |
| 12:17.43 | *** join/#asterisk-dev italorossi (~italoross@187.60.66.11) |
| 12:22.38 | *** join/#asterisk-dev protocoldoug (~quassel@unaffiliated/protocoldoug) |
| 12:39.50 | *** join/#asterisk-dev danjenkins_ (~dan@cpc65687-folk2-2-0-cust207.1-2.cable.virginmedia.com) |
| 13:07.22 | *** join/#asterisk-dev [TK]D-Fender (~chatzilla@216-191-106-163.dedicated.allstream.net) |
| 13:45.30 | *** join/#asterisk-dev danjenkins (~dan@cpc65687-folk2-2-0-cust207.1-2.cable.virginmedia.com) |
| 13:49.20 | *** join/#asterisk-dev oej (~Adium@h87-96-134-129.dynamic.se.alltele.net) |
| 13:49.21 | *** mode/#asterisk-dev [+o oej] by ChanServ |
| 13:49.51 | *** join/#asterisk-dev putnopvut (~putnopvut@asterisk/master-of-queues/mmichelson) |
| 13:49.51 | *** mode/#asterisk-dev [+o putnopvut] by ChanServ |
| 14:07.33 | *** join/#asterisk-dev sgalarneau (~Adium@nat/digium/x-kaszojiyqxjrrbpg) |
| 14:09.24 | *** join/#asterisk-dev mjordan (~matt@nat/digium/x-hkjpbmcwssimhbpy) |
| 14:09.25 | *** mode/#asterisk-dev [+o mjordan] by ChanServ |
| 14:12.26 | *** join/#asterisk-dev rmudgett (~rmudgett@nat/digium/x-ndecpgcmbaxebdwe) |
| 14:23.08 | *** join/#asterisk-dev kharwell (~kharwell@nat/digium/x-fxlerlwlgkgnnvbi) |
| 14:28.44 | mjordan | opticron: so, about the whole 'no more implicit subscription when you create a bridge' thing |
| 14:29.12 | mjordan | sgalarneau: leedm777: I'd be interested in your take on this as well |
| 14:29.17 | mjordan | is this a breaking change? |
| 14:31.04 | sgalarneau | mjordan: I thought getting bridge events required an explicit subscription |
| 14:31.52 | file | it does require an explicit subscription |
| 14:32.50 | mjordan | hm |
| 14:33.19 | mjordan | well, the difference is that when a channel entered a bridge, the subscription created as a result of the channel in the app being in the bridge persisted |
| 14:33.34 | mjordan | now, when that channel leaves the bridge, you stop getting bridge events (since you never subscribed to it in the first place) |
| 14:33.43 | file | correct |
| 14:34.09 | sgalarneau | so adding a channel to a bridge in a Stasis app always implicitly subscribes to bridge events? |
| 14:34.10 | mjordan | which, while wrong, was still something we were sort of depending on |
| 14:34.19 | mjordan | sgalarneau: it did. It shouldn't have. It doesn't any more |
| 14:34.33 | file | well, hold up |
| 14:35.37 | file | does it still do that OR is the fact that the subscription wasn't going away when the channel should have no longer been subscribed fixed, cause that's two different things |
| 14:35.49 | sgalarneau | so this is technically separate from the bridge creation issue where you might miss events between bridge creation and subscription |
| 14:36.37 | mjordan | file: yeah, I should clarify. You still get the bridge subscription when the channel is in there (you better, anyway). It just goes away when the channel leaves |
| 14:36.54 | sgalarneau | that makes sense from a conceptual level at least |
| 14:37.07 | mjordan | sgalarneau: correct. The fact that you aren't subscribed to a bridge you create is really a separate problem, and is basically an artifact of a bridge not really being an automatic 'member' of a Stasis application |
| 14:37.19 | file | good, just making sure I didn't miss something crazy |
| 14:37.33 | mjordan | I'm just going to make a note in UPGRADE |
| 14:37.55 | mjordan | we have to bump the version of ARI anyway, just wanted to make sure we didn't view this as a breaking change (which would require a major version bump) |
| 14:38.10 | mjordan | it is *kind* of breaking, but the API itself didn't break.... kind of. |
| 14:38.29 | file | it's breaking in the sense we fixed a bug which created behavior that people may have relied on, albeit undocumented behavior |
| 14:38.53 | sgalarneau | if all Stasis channels leave a bridge that wasn't created by a Stasis app, I would expect to no longer get events for that bridge, and if we eventually make it so bridge creation from a Stasis app can subscribe to events along with creation that shouldn't be an issue |
| 14:39.41 | sgalarneau | would we run into problems if we auto subscribed along with bridge creation and all channels left the bridge? Because I would expect to still get bridge events in that case. |
| 14:39.55 | file | nope |
| 14:40.09 | file | you'd still have a subscription, until you deleted the bridge |
| 14:40.55 | sgalarneau | so it's really only a braking change in the sense that some users of ARI might expect that behavior which is in fact a bug |
| 14:41.02 | file | correct |
| 14:41.18 | sgriepentrog | But the bridge creation ari request would have to mention the stasis app by name in order to enable subscription to the bridge. |
| 14:41.27 | file | automatically, yes |
| 14:42.10 | mjordan | right, that's what opticron's e-mail to the app-dev list was suggesting |
| 14:42.11 | sgalarneau | we already do that on originate, it's no different |
| 14:42.42 | file | affirmative |
| 14:43.45 | sgalarneau | that's consistent, might even be able to abstract it out in ARI libraries |
| 14:44.14 | sgalarneau | although we'd no longer be transparent about what's happening on originate or bridge creation |
| 14:45.55 | *** join/#asterisk-dev otherwiseguy (~otherwise@2605:a601:43e:3501:2677:3ff:fe8f:2730) |
| 14:46.01 | mjordan | rmudgett: is there anything that should be added to the UPGRADE file for the IAX2 frame issue (where different versions of Asterisk using chan_iax2 would have hangups due to mismatched control frames)? |
| 14:46.58 | rmudgett | Wasn't there a note added already? |
| 14:51.37 | rmudgett | mjordan: The iax.conf.sample file was updated. No change was done to UPGRADE or CHANGES. |
| 14:52.48 | mjordan | rmudgett: I'm going to put a note into the UPGRADE file for 1.8+ |
| 14:54.32 | *** join/#asterisk-dev tzafrir (~tzafrir@bzq-84-109-18-138.red.bezeqint.net) |
| 14:54.43 | *** mode/#asterisk-dev [+o tzafrir] by ChanServ |
| 15:24.06 | wdoekes | mjordan: was there a rush with 3375? |
| 15:44.57 | mjordan | wdoekes: No, was just merging in community patches with a Ship It |
| 15:46.11 | *** join/#asterisk-dev sgalarneau (~Adium@nat/digium/x-kvgbiyvsbbemmjwd) |
| 15:52.30 | wdoekes | ok, because that should get a version for 12 in as well |
| 15:53.22 | mjordan | wdoekes: yeah, I just saw zvision's comment. The two reviews had the version for 12 in the one for trunk, hence why I blocked it |
| 15:54.16 | wdoekes | indeed, the 11 version should get forward ported to 12, and the trunk one be separate |
| 15:54.47 | mjordan | k, I'll merge the 11 one on up then. |
| 15:55.10 | wdoekes | alrighty |
| 15:58.12 | *** join/#asterisk-dev gtjoseph (~gtj0@unaffiliated/gtj) |
| 16:26.10 | mjordan | wdoekes: and with your Ship It, I'll merge the trunk one as well shortly |
| 16:26.19 | mjordan | yay merge conflicts going from 11 to 12 :-P |
| 16:30.41 | rmudgett | Does the testsuite have a test that uses the AMI Originate action to originate calls? I'm only able to find examples of sending a CLI originate for calls. |
| 16:38.47 | mjordan | rmudgett: The testsuite uses AMI to originate all sorts of things. IIRC, the Originator plugin uses it |
| 16:39.12 | mjordan | putnopvut: do we need to mention anything about the user_agent field being added to the pjsip contact object? |
| 16:39.18 | rmudgett | mjordan: I was having some difficulty finding actual uses. I just found one. |
| 16:40.23 | file | mjordan, for cases where people may be managing their own table... I suppose |
| 16:40.37 | mjordan | yay. Which now leads me back to... Alembic. |
| 16:41.02 | mjordan | but, that's in the UPGRADE |
| 16:41.03 | mjordan | huzzah |
| 16:42.01 | putnopvut | mjordan: yeah, mentioning user_agent is a Good Thing(tm) |
| 16:42.02 | mjordan | er |
| 16:42.08 | mjordan | hm. It isn't in Alembci. |
| 16:42.09 | mjordan | Alembic. |
| 16:43.05 | putnopvut | mjordan: it should be. 8b89128752a_add_user_agent.py |
| 16:43.45 | putnopvut | aw dammit, I didn't svn add it when I committed other user agent stuff. |
| 16:44.15 | putnopvut | my bad |
| 16:44.25 | mjordan | heh |
| 16:44.33 | mjordan | I was looking for it and not seeing it... |
| 16:45.09 | mjordan | yay! Not crazy |
| 16:45.15 | putnopvut | So in order not to totally screw alembic up, I'll need to recreate the revision. |
| 16:46.49 | sgriepentrog | Okay, so I know I just committed the shutdown() fix for http, but Josh's TCP_NODELAY also fixes the problem... |
| 16:47.29 | sgriepentrog | The question is, what is the better way of approaching this, and oh, btw, I probably didn't need to patch manager since it's already using no delay? |
| 16:48.32 | file | without knowing why... there is no right or wrong |
| 16:49.10 | putnopvut | Has the alembic multiple heads thing not been fixed yet? |
| 16:49.16 | sgriepentrog | It's still a bug in tcp buffer handling, if close() doesn't kick the last data out sometimes. |
| 16:49.24 | putnopvut | Because after updating 12, I still can't add a new alembic revision. |
| 16:49.36 | file | putnopvut, I thought I saw rmudgett fix it... |
| 16:51.12 | rmudgett | putnopvut: I fixed it. |
| 16:53.50 | putnopvut | http://ur1.ca/gxn45 |
| 17:01.55 | mjordan | sgriepentrog: the only benefit of going with file's approach is we would be consistent across both manager and thtp |
| 17:01.56 | putnopvut | Oh I see one of those is the "add_user_agent" thing. That needs to be removed. |
| 17:01.56 | mjordan | er |
| 17:01.57 | mjordan | http |
| 17:02.16 | coreyf1513 | sgriepentrog: between TCP_NODELAY and shutdown, if we're trying to solve "lost data at time of TCP close", I prefer shutdown over of TCP_NODELAY. shutdown only adds slight overhead when we're done with the connection. |
| 17:02.25 | rmudgett | putnopvut: add_user_agent is conflicting. See https://wiki.asterisk.org/wiki/display/AST/Merging+alembic+scripts |
| 17:03.18 | sgriepentrog | Okay, that's a good point. TCP_NODELAY means loosing buffering efficiency, although the slight speed improvement could be argued to be beneficial to ari? |
| 17:04.08 | coreyf1513 | sgriepentrog: Is ARI latency is more important than ARI throughput? |
| 17:04.49 | putnopvut | rmudgett: yeah, I got it figured out, thanks :) |
| 17:05.10 | sgriepentrog | I would say if you're doing ARI on same-host or on same subnet (no router) then low latency is desired. But if you're doing it across longer distances, you favor throughput. |
| 17:06.42 | sgriepentrog | Another way to look at it though is that shutdown() is only a change at the end of the sequence - and thus "less" of a change than turning on nodelay. |
| 17:07.12 | sgriepentrog | Even though manager is using NODELAY, I'm thinking that ARI shouldn't. |
| 17:07.26 | file | dwelling on that is silly, keep it at shutdown |
| 17:07.42 | sgriepentrog | Should we leave the shutdown() in manager too, or drop that? |
| 17:07.48 | *** join/#asterisk-dev sgalarneau (~Adium@nat/digium/x-jptzdaivgijxyobe) |
| 17:10.35 | file | it won't harm things. |
| 17:33.58 | *** join/#asterisk-dev scouture (~scouture@unaffiliated/scouture) |
| 17:41.46 | *** join/#asterisk-dev Looserouting (~looserout@p5B13746C.dip0.t-ipconnect.de) |
| 17:52.30 | *** join/#asterisk-dev sgalarneau (~Adium@nat/digium/x-trtadlsfdxmsgbfe) |
| 17:56.21 | mjordan | FYI: I'm going to start the release process in 30 min |
| 17:56.47 | *** join/#asterisk-dev pc-m (~pcm@modemcable094.94-70-69.static.videotron.ca) |
| 17:58.56 | *** join/#asterisk-dev pc-m (~pcm@modemcable094.94-70-69.static.videotron.ca) |
| 17:59.41 | Looserouting | hi. is there interest in adding rtp stats to cdr ? |
| 18:00.05 | mjordan | nope! |
| 18:00.24 | mjordan | (1) You could already do that using the various CHANNEL functions that access RTCP statistics and the CDR function |
| 18:00.53 | mjordan | (2) CDRs should not be a general catch all for everything you want to slam in there. If you want it, you can using the adaptive backends - but that does not mean that everyone will want that |
| 18:01.10 | mjordan | (3) Olle (oej) has a branch (pinefrog) that creates Call Quality Records, which are more in line with what you're proposing. |
| 18:01.34 | mjordan | If anything, something closer to CQR would be preferred, which would not overburden the existing CDR mechanisms. |
| 18:01.52 | mjordan | and that's assuming you don't just roll your own reporting mechanisms using AMI and the RTCP* events |
| 18:02.00 | mjordan | so, nope. |
| 18:02.40 | *** join/#asterisk-dev danjenkins_ (~dan@cpc65687-folk2-2-0-cust207.1-2.cable.virginmedia.com) |
| 18:02.56 | Looserouting | ok thanks. |
| 18:03.00 | mjordan | np! |
| 18:03.30 | *** join/#asterisk-dev pc-m (~pcm@modemcable094.94-70-69.static.videotron.ca) |
| 18:05.32 | *** join/#asterisk-dev pc-m (~pcm@modemcable094.94-70-69.static.videotron.ca) |
| 18:52.39 | *** join/#asterisk-dev danjenkins (~dan@cpc65687-folk2-2-0-cust207.1-2.cable.virginmedia.com) |
| 19:13.19 | coreyf1513 | gtjoseph: ping |
| 19:54.24 | gtjoseph | coreyf1513: I'm back. |
| 19:55.27 | coreyf1513 | I was reading up on spinlocks, I think we need to make sure we use mutex if the system is not multicore |
| 19:57.15 | gtjoseph | Yeah, it's possible. I was going to look at how the kernel does it un a UP situation. |
| 19:57.31 | coreyf1513 | on a multicore system the other thread that has the spinlock will release it from a different CPU, thus no context switch needed. if you only have one CPU, a spinlock burns CPU cycles until the OS does the normal scheduler switch |
| 19:59.15 | coreyf1513 | it's possible that pthread_spinlock is smart enough to know when you are UP, but I don't know if that assumption can be made cross-platform. I would say gcc atomics can only be used if we are 100% that we're not UP |
| 19:59.23 | gtjoseph | I'm going to run the tests on UP Atom and Arm machines and see what happens |
| 20:00.20 | coreyf1513 | I would say the other big platform variable is virtual machines |
| 20:01.23 | gtjoseph | Yep. I was going to test on my own vms and my ec2 and rackspace vms as well. |
| 20:02.19 | gtjoseph | right now i'm testing a gas version using xchgl directly. |
| 20:02.47 | coreyf1513 | are you going to test with 1 and 2 virtual CPU's? |
| 20:03.45 | gtjoseph | yes and also with smp disabled in the kernel |
| 20:04.31 | gtjoseph | it's going to be a busy weekend :) |
| 20:26.20 | coreyf1513 | gtjoseph: sorry to add work but I think it's important to make sure this doesn't make performance bad for people (and make it better for some people). |
| 20:26.51 | gtjoseph | agree 100% |
| 20:27.33 | coreyf1513 | do you have any specific use cases in mind? |
| 20:28.03 | gtjoseph | yep, the lock around an astobj2 refcount. |
| 20:28.17 | gtjoseph | which means it has to be VERY speedy. |
| 20:28.34 | gtjoseph | and no slower than the current implementation |
| 20:30.51 | gtjoseph | what I really wish is some data around asterisk installations... what percentage on which oses, which machine architectures. |
| 20:31.19 | coreyf1513 | how much do you plan to have in the refcount lock? |
| 20:31.55 | gtjoseph | a test and set are all that's needed. |
| 20:32.09 | gtjoseph | which is why the spinlock is a candate |
| 20:32.13 | gtjoseph | candidate |
| 20:32.20 | gtjoseph | and mutex is not |
| 20:32.32 | mjordan | gtjoseph: you aren't the only one who wishes he had that data... |
| 20:33.11 | gtjoseph | Maybe we can get the NSA to collect it for us :) |
| 20:33.21 | coreyf1513 | but if mutex is not a candidate, then spinlock is also not a candidate - we can't abandon uni-processor people, and for people with uni-processor they would effectively be using a mutex or worse |
| 20:34.02 | gtjoseph | spinlocks were around before MP so there's a solution. |
| 20:34.46 | gtjoseph | the only thing needed really is the atomic test and swap |
| 20:36.37 | coreyf1513 | gtjoseph: care to pastebin something in C for to show what you plan inside a spinlock for ao2_ref? sorry I'm still not really following |
| 20:36.54 | gtjoseph | yep gimme a few... |
| 20:41.26 | gtjoseph | coreyf1513: http://pastebin.com/kXCVS4C6 |
| 20:52.56 | coreyf1513 | gtjoseph: This would probably make it possible for weak references to be implemented safely, but I'm concerned with serializing ref counting. I'm not sure others will go for any locking within ao2_ref. |
| 20:53.29 | gtjoseph | it's already protected by an atomic fetch and add. |
| 20:53.49 | gtjoseph | in the case of a strong reffed object, a spinlock is no more than that |
| 20:53.59 | gtjoseph | because there can't be a lock |
| 20:55.20 | gtjoseph | that's why all the testing,... to make sure that the default strong ref implementation doesn't get penalized. |
| 20:58.09 | coreyf1513 | atomic fetch and add is not a lock normally. you had explained what you wanted weak references for once, was that on the old review? do you have a link to that? I'd like to take a look at what you want weak references for |
| 21:02.38 | *** join/#asterisk-dev [TK]D-Fender (~chatzilla@64.235.216.2) |
| 21:03.11 | gtjoseph | https://reviewboard.asterisk.org/r/3382/ The idea was to allow a container to contain an object without holding a ref on it this way when the last user of the object is donw with it, it's destructor would clean it ouf of the container |
| 21:04.06 | gtjoseph | as you pointed out, this raises the possibility that 2 users might unref an object to 0 at the same time. |
| 21:07.54 | coreyf1513 | let me think on this for a bit, are you around later? |
| 21:08.10 | gtjoseph | oh yeah. Only 3pm my time. |
| 21:08.49 | coreyf1513 | cool I'm going to eat and think about this, I'll be back |
| 21:09.00 | *** join/#asterisk-dev otherwiseguy (~otherwise@69.23.159.253) |
| 21:14.27 | *** join/#asterisk-dev danjenkins (~dan@cpc65687-folk2-2-0-cust207.1-2.cable.virginmedia.com) |
| 21:16.53 | *** join/#asterisk-dev danjenkins (~dan@cpc65687-folk2-2-0-cust207.1-2.cable.virginmedia.com) |
| 22:07.37 | *** join/#asterisk-dev mjordan (~matt@nat/digium/x-rklhnzyhqftwtupv) |
| 22:07.38 | *** mode/#asterisk-dev [+o mjordan] by ChanServ |
| 22:32.10 | *** part/#asterisk-dev kharwell (~kharwell@nat/digium/x-fxlerlwlgkgnnvbi) |
| 22:38.00 | coreyf1513 | gtjoseph: I think I have an idea, I'm not 100% sure yet that it's safe, or acceptable (it adds an int and a pointer to __priv_data). |
| 22:38.15 | gtjoseph | yeeees... |
| 22:38.24 | coreyf1513 | gtjoseph: Create an ao2_alloc_disposable constructor for use by objects like your sorcery wizard. This constructor would accept two extra parameters, one for a 'low-water' refcount, another for dispose_fn callback. |
| 22:38.42 | coreyf1513 | ao2_ref would call your dispose_fn anytime refcount is decreased to equal low-water mark. Your dispose_fn would lock the container. If the object is still at low-water mark remove it from the container. |
| 22:39.50 | coreyf1513 | just something I worked up. I could have not been able to poke holes in the concept, but that could change once it's actual code |
| 22:41.01 | gtjoseph | that's an option but it requires that you know the object is going into a weak container at the time you create the object. |
| 22:41.36 | gtjoseph | and an object wouldn't be able to be in both a weak container and a strong one at the same time |
| 22:41.45 | coreyf1513 | yes it would |
| 22:42.07 | coreyf1513 | there actually would be no concept of weak container |
| 22:42.34 | coreyf1513 | the container would be strong, and get a strong reference to the object. the object would have a low-water mark of 1, causing the dispose_fn to run when the only reference is the container |
| 22:42.45 | gtjoseph | but again, the behavior is tied to the object |
| 22:43.32 | gtjoseph | which is kind of the reverse of the weak reference concept |
| 22:44.41 | gtjoseph | the concept is that the holder decides whether the reference should be weak or strong, not the holdee. |
| 22:44.54 | coreyf1513 | well I wasn't actually looking for a way to implement weak references, I was looking for a way to have a cachable object in an ao2_container auto-clean itself, without any safety issues |
| 22:45.30 | gtjoseph | hmmm. might still work. |
| 22:47.40 | gtjoseph | a possible issue... wouldn't the object need to store pointers back to the containers it's in? I do that for weak containers but this would require all containers to be stored. |
| 22:47.42 | roxlu | hey guys, maybe someone around with knowlegde about srtp? I'm receiving srtp and decrypting them, and now I'm wondering what would be a way to extract the actual video data? |
| 22:51.46 | coreyf1513 | gtjoseph: you're right ,you would want the object to keep a strong reference to the container. once the object is disposed, it would release the reference to the container. this would prevent the container from being destroyed before all objects, but if the container goes first that would be bad |
| 22:52.09 | mjordan | roxlu: once you've decrypted the data, its an RTP packet |
| 22:52.42 | roxlu | mjordan: i'm a super noob with this :# |
| 22:52.55 | mjordan | gtjoseph: so, I've been reading through the conversation, but I haven't looked at your weak ref code yet |
| 22:53.10 | mjordan | the way I was previously thinking of weak refs was to use them as their own object |
| 22:53.17 | mjordan | really, just as handle to some other object |
| 22:53.26 | mjordan | and not as the object themselves |
| 22:53.33 | mjordan | that seems to be the path corey is going down (unless I'm mistaken) |
| 22:53.41 | coreyf1513 | mjordan: that sounds like how firefox does it: https://developer.mozilla.org/en-US/docs/Weak_reference |
| 22:53.45 | mjordan | yup |
| 22:53.59 | coreyf1513 | that was my original suggestion |
| 22:54.03 | mjordan | I've seen some implementations in .NET that do that for events as well (scary, I know) |
| 22:54.06 | gtjoseph | yeah, that's the proxy pattern |
| 22:54.10 | mjordan | yeup |
| 22:54.19 | gtjoseph | but it's not exactly light weight |
| 22:54.22 | mjordan | nope |
| 22:54.23 | mjordan | :-) |
| 22:54.31 | gtjoseph | i was trying to minimize the creation of new objects |
| 22:54.56 | coreyf1513 | mjordan: just now I was talking about adding a 'low-water' mark for references, and a dispose_fn callback to be run when we decrease to low-water references |
| 22:55.07 | mjordan | understandable. Often though a weak reference is really someone wanting to have access to a strongly referenced object without impacting its lifetime, so I'm not sure how you'd make the strongly referenced object both strong and weak |
| 22:55.12 | mjordan | hm |
| 22:55.24 | mjordan | coreyf1513: that makes more sense now. |
| 22:55.33 | mjordan | The one thing to think about with all of this is how complex we want to make ao2 |
| 22:55.50 | coreyf1513 | so you would run: ao2_alloc_disposable(size_t sz, int low_water, ao2_dispose_fn dispose_cb) |
| 22:55.57 | mjordan | one thing I think we partially regret was having it get as complex as it is now - in performance testing, we've definitely seen some cache misses with the virtual table callbacks, and other things |
| 22:56.20 | mjordan | but that's just something to keep in the back of your head - to be frank, I haven't looked at anything to have a strong enough opinion yet ;-) |
| 22:58.19 | coreyf1513 | I was just putting the last idea out as I understand the problem gtjoseph is having, but think that avoiding weak references is a good thing |
| 22:59.31 | gtjoseph | coreyf1513: Not following on why weak references aren't a good idea. |
| 22:59.52 | mjordan | there have been other times when we wished we had weak refs |
| 23:00.00 | mjordan | there's actually a few places in the code that would benefit from that |
| 23:00.20 | mjordan | whenever you see something checking to see how many refs are left and then decrement the count multiple times - that's a candidate for a weak ref :-) |
| 23:00.38 | *** part/#asterisk-dev mjordan (~matt@nat/digium/x-rklhnzyhqftwtupv) |
| 23:00.40 | coreyf1513 | gtjoseph: it violates a very basic rule of ao2, which is to always have a reference to an ao2 object before you do any operation on it (including get another reference) |
| 23:01.27 | gtjoseph | that doesn't mean we can't change the rule in very controlled circumstances |
| 23:02.41 | gtjoseph | if the implementation is technically sound then it's up to the user (developer) to decide which model is appropriate in which circumstances. |
| 23:35.39 | *** join/#asterisk-dev tzafrir (~tzafrir@bzq-84-109-18-138.red.bezeqint.net) |
| 23:35.39 | *** mode/#asterisk-dev [+o tzafrir] by ChanServ |