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

reference count is logged in as undefined, and the channel cannot be GC'ed though #69

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

andreip
Copy link
Contributor

@andreip andreip commented Mar 22, 2013

Some context:

  • channel is created, but no widget uses it, so ref count is undefined, how is initialized in newDataChannels.
  • time_of_reference_expiry is null when the channel is first created
  • in gc.coffee:channelCanBeGarbageCollected, this piece of code will activate, because the time_of_reference_expiry is null
# If this channel still has references attached, so skip it.
unless reference['time_of_reference_expiry']?
    logger.info("Channel #{channel} cannot be garbage " +
                    "collected because it still has widgets " +
                    "referencing it. (reference count: #{reference.reference_count})")
    return false

so the channel will never have time_of_reference_expiry thus will not advance to return true to GC the channel.

@ghost ghost assigned andreip Mar 22, 2013
@andreip
Copy link
Contributor Author

andreip commented Apr 2, 2013

@arcade
Copy link
Contributor

arcade commented Apr 3, 2013

We should never initialize channels of no widget references them, since that's just a waste of both memory and CPU cycles (while garbage collecting them on the next tick).

I'm not sure why we do this but if we really have to, we should do a lazy initialization, and only initialize the channel when someone actually needs it (newDataChannel should return a Lazy Promise that should be resolved when someone actually calls then on it). That way, we keep the gc happy, and the memory footprint down.

@aismail
Copy link
Contributor

aismail commented Apr 3, 2013

There's an issue for that already in our internal repo, but never got to do
it. Let's search for it and revive it by moving the discussion in the open
:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants