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

revert netsplit print optimisation #812

Merged
merged 1 commit into from Jan 14, 2018

Conversation

Projects
None yet
2 participants
@ailin-nemui
Contributor

ailin-nemui commented Jan 10, 2018

this reverts part of #465

unfortunately we need to further refine the initial patch

  • when filtering by channel, the whole split is cleaned up nevertheless
  • something similar happens for the netjoins
  • furthermore, we cannot wait only for PUBLIC msgs, j/p/q are equivalently relevant for temporal integrity

this is a temporary band aid until we can properly implement #465 and f.x #420. the basic idea presented therein seems sound

this is intended to fix #809

revert netsplit print optimisation
this reverts part of #465

unfortunately we need to further refine the initial patch

 - when filtering by channel, the whole split is cleaned up nevertheless
 - something similar happens for the netjoins
 - furthermore, we cannot wait only for PUBLIC msgs, j/p/q are equivalently relevant for temporal integrity
@dequis

This comment has been minimized.

Member

dequis commented Jan 10, 2018

Yeah sounds like the sanest thing to do. Is this going in 1.0.x and 1.1.0?

@dequis

This comment has been minimized.

Member

dequis commented Jan 10, 2018

Oh, uh, this needs review too as it's a partial revert so that could bring problems of its own too.

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Jan 10, 2018

I would include it in a potential 1.0.7 and ideally in 1.1.0. we probably won't be able to land a complete fix for this in either 1.0 or 1.1 because too much rewrite

@dequis

This comment has been minimized.

Member

dequis commented Jan 12, 2018

So, relevant commits:

  • #465 "Some small adjustments to the netsplit code"
    • ed06e43
    • 8f5e200 "Avoid entering an endless loop while traversing the channel list"
    • c0f66c9 "how did we allow ourselves to get this commit message merged"
  • #577 "Minor corrections to the netsplit code"
    • 7574bed
    • 5f0e755 "Don't shadow the 'channel' variable when printing the netjoins."

Cherry picked the last two on top of the other three and made this branch for easy diffing of the whole thing:

dequis/irssi@lemon-netsplit-base...dequis:cherry-picked-lemon-netsplit-stuff

Other commits touching fe-netjoin.c or fe-netsplit.c afterwards:

  • 98ead50 "Prevent some potential null-pointer deferences" (GL!9)
  • 7c09b72 "fe-netjoin: remove irc servers on "server disconnected" signal" (GL#7, GL!10)

Both look like crash fixes so probably not relevant.

@dequis

This comment has been minimized.

Member

dequis commented Jan 12, 2018

Things that aren't getting reverted from fe-netsplit:

  • The old sig_print_starting didn't look at dest, and iterated over all servers.
  • The server_ischannel(dest->server, dest->target)

Things that aren't getting reverted from fe-netjoin:

  • The cleaner iteration style, with g_slist_next + g_slist_delete_link
  • Same stuff as above

Mostly harmless stuff. Any particular reason to leave the server_ischannel in? Going back to iterating over all servers might be the safest option too, but that's probably too much and won't change things meaningfully. On the other hand leaving these things in gives us some bits of data, so,

I can't see this making things worse, at least.

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Jan 12, 2018

thanks for the review / research, maybe we should just completely revert? at least by checking the server 8 hope maybe we can have the trackbar<>netsplit printing fixed, but I couldn't verify that

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Jan 12, 2018

@ailin-nemui ailin-nemui merged commit 7de1378 into irssi:master Jan 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ailin-nemui ailin-nemui deleted the ailin-nemui:tape-netsplit branch Jan 21, 2018

lkundrak pushed a commit to lkundrak/irssi that referenced this pull request Feb 16, 2018

Merge pull request irssi#812 from ailin-nemui/tape-netsplit
revert netsplit print optimisation

(cherry picked from commit 7de1378)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment