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

[RFC] possible rstream simplification #980

Closed
aktau opened this issue Jul 22, 2014 · 4 comments
Closed

[RFC] possible rstream simplification #980

aktau opened this issue Jul 22, 2014 · 4 comments
Labels
refactor changes that are not features or bugfixes

Comments

@aktau
Copy link
Contributor

aktau commented Jul 22, 2014

@tarruda some time ago I asked a question about the possibility of alloc_cb(); alloc_cb(); read_cb(); read_cb() occurring on Windows: https://groups.google.com/forum/#!topic/libuv/OD38PeGeVgQ

This answer is enlightening:

E.g. this could happen: 

> > alloc_cb(handle1); 
> > alloc_cb(handle2); 
> > read_cb(handle1); 
> > read_cb(handle2); 

But this isn't: 

> > alloc_cb(handle1); 
> > alloc_cb(handle1); 
> > read_cb(handle1); 
> > read_cb(handle1); 

While reading rstreamc., I see:

// Called by libuv to allocate memory for reading.
static void alloc_cb(uv_handle_t *handle, size_t suggested, uv_buf_t *buf)
{
  RStream *rstream = handle_get_rstream(handle);

  if (rstream->reading) {
    buf->len = 0;
    return;
  }

  buf->len = rstream->buffer_size - rstream->wpos;
  buf->base = rstream->buffer + rstream->wpos;

  // Avoid `alloc_cb`, `alloc_cb` sequences on windows
  rstream->reading = true;
}

It seems like the rstream->reading thing is not necessary, even on Windows. The reason being that each stream has a 1-to-1 correspondance with a libuv handle, and alloc_cb(handle1); alloc_cb(handle1); can never happen.

There would be a problem if we were reusing the same rstream->buffer for all streams though. As far as I can see, we don't though.

@tarruda
Copy link
Member

tarruda commented Jul 22, 2014

I added this guard is because @saghul mentioned(or I misunderstood) that sequential calls to alloc_cb might happen for the same stream on windows.

@aktau
Copy link
Contributor Author

aktau commented Jul 22, 2014

I added this guard is because @saghul mentioned(or I misunderstood) that sequential calls to alloc_cb might happen for the same stream on windows.

Do you remember where he said it? I'd like to know for sure too.

@saghul
Copy link

saghul commented Jul 22, 2014

I added this guard is because @saghul mentioned(or I misunderstood) that sequential calls to alloc_cb might happen for the same stream on windows.

Not for the same stream, just for different streams.

@aktau
Copy link
Contributor Author

aktau commented Jul 22, 2014

Not for the same stream, just for different streams.

Perfect, then we can remove a couple of lines. I'll do it in my PR close to the commit that increases the buffer size. (and thanks @saghul!)

aktau added a commit to aktau/neovim that referenced this issue Jul 22, 2014
Not necessary, as discussed in neovim#980.

Closes neovim#980.
aktau added a commit to aktau/neovim that referenced this issue Jul 22, 2014
Not necessary, as discussed in neovim#980.

Closes neovim#980.
aktau added a commit to aktau/neovim that referenced this issue Jul 22, 2014
Not necessary, as discussed in neovim#980.

From the libuv mailing list:
https://groups.google.com/forum/#!topic/libuv/OD38PeGeVgQ

E.g. this could happen (red: on Windows):

> > alloc_cb(handle1);
> > alloc_cb(handle2);
> > read_cb(handle1);
> > read_cb(handle2);

But this couldn't:

> > alloc_cb(handle1);
> > alloc_cb(handle1);
> > read_cb(handle1);
> > read_cb(handle1);

Because each stream has a 1-to-1 correspondance with a libuv handle. The
code removed was never executed.

Closes neovim#980.
aktau added a commit to aktau/neovim that referenced this issue Jul 23, 2014
Not necessary, as discussed in neovim#980.

From the libuv mailing list:
https://groups.google.com/forum/#!topic/libuv/OD38PeGeVgQ

E.g. this could happen (red: on Windows):

> > alloc_cb(handle1);
> > alloc_cb(handle2);
> > read_cb(handle1);
> > read_cb(handle2);

But this couldn't:

> > alloc_cb(handle1);
> > alloc_cb(handle1);
> > read_cb(handle1);
> > read_cb(handle1);

Because each stream has a 1-to-1 correspondance with a libuv handle. The
code removed was never executed.

Closes neovim#980.
aktau added a commit to aktau/neovim that referenced this issue Jul 23, 2014
Not necessary, as discussed in neovim#980.

From the libuv mailing list:
https://groups.google.com/forum/#!topic/libuv/OD38PeGeVgQ

E.g. this could happen (red: on Windows):

> > alloc_cb(handle1);
> > alloc_cb(handle2);
> > read_cb(handle1);
> > read_cb(handle2);

But this couldn't:

> > alloc_cb(handle1);
> > alloc_cb(handle1);
> > read_cb(handle1);
> > read_cb(handle1);

Because each stream has a 1-to-1 correspondance with a libuv handle. The
code removed was never executed.

Closes neovim#980.
aktau added a commit to aktau/neovim that referenced this issue Jul 24, 2014
Not necessary, as discussed in neovim#980.

From the libuv mailing list:
https://groups.google.com/forum/#!topic/libuv/OD38PeGeVgQ

E.g. this could happen (red: on Windows):

> > alloc_cb(handle1);
> > alloc_cb(handle2);
> > read_cb(handle1);
> > read_cb(handle2);

But this couldn't:

> > alloc_cb(handle1);
> > alloc_cb(handle1);
> > read_cb(handle1);
> > read_cb(handle1);

Because each stream has a 1-to-1 correspondance with a libuv handle. The
code removed was never executed.

Closes neovim#980.
aktau added a commit to aktau/neovim that referenced this issue Jul 24, 2014
Not necessary, as discussed in neovim#980.

From the libuv mailing list:
https://groups.google.com/forum/#!topic/libuv/OD38PeGeVgQ

E.g. this could happen (red: on Windows):

> > alloc_cb(handle1);
> > alloc_cb(handle2);
> > read_cb(handle1);
> > read_cb(handle2);

But this couldn't:

> > alloc_cb(handle1);
> > alloc_cb(handle1);
> > read_cb(handle1);
> > read_cb(handle1);

Because each stream has a 1-to-1 correspondance with a libuv handle. The
code removed was never executed.

Closes neovim#980.
aktau added a commit to aktau/neovim that referenced this issue Jul 24, 2014
Not necessary, as discussed in neovim#980.

From the libuv mailing list:
https://groups.google.com/forum/#!topic/libuv/OD38PeGeVgQ

E.g. this could happen (red: on Windows):

> > alloc_cb(handle1);
> > alloc_cb(handle2);
> > read_cb(handle1);
> > read_cb(handle2);

But this couldn't:

> > alloc_cb(handle1);
> > alloc_cb(handle1);
> > read_cb(handle1);
> > read_cb(handle1);

Because each stream has a 1-to-1 correspondance with a libuv handle. The
code removed was never executed.

Closes neovim#980.
aktau added a commit to aktau/neovim that referenced this issue Jul 24, 2014
Not necessary, as discussed in neovim#980.

From the libuv mailing list:
https://groups.google.com/forum/#!topic/libuv/OD38PeGeVgQ

E.g. this could happen (red: on Windows):

> > alloc_cb(handle1);
> > alloc_cb(handle2);
> > read_cb(handle1);
> > read_cb(handle2);

But this couldn't:

> > alloc_cb(handle1);
> > alloc_cb(handle1);
> > read_cb(handle1);
> > read_cb(handle1);

Because each stream has a 1-to-1 correspondance with a libuv handle. The
code removed was never executed.

Closes neovim#980.
aktau added a commit to aktau/neovim that referenced this issue Jul 27, 2014
Not necessary, as discussed in neovim#980.

From the libuv mailing list:
https://groups.google.com/forum/#!topic/libuv/OD38PeGeVgQ

E.g. this could happen (red: on Windows):

> > alloc_cb(handle1);
> > alloc_cb(handle2);
> > read_cb(handle1);
> > read_cb(handle2);

But this couldn't:

> > alloc_cb(handle1);
> > alloc_cb(handle1);
> > read_cb(handle1);
> > read_cb(handle1);

Because each stream has a 1-to-1 correspondance with a libuv handle. The
code removed was never executed.

Closes neovim#980.
aktau added a commit to aktau/neovim that referenced this issue Jul 27, 2014
Not necessary, as discussed in neovim#980.

From the libuv mailing list:
https://groups.google.com/forum/#!topic/libuv/OD38PeGeVgQ

E.g. this could happen (red: on Windows):

> > alloc_cb(handle1);
> > alloc_cb(handle2);
> > read_cb(handle1);
> > read_cb(handle2);

But this couldn't:

> > alloc_cb(handle1);
> > alloc_cb(handle1);
> > read_cb(handle1);
> > read_cb(handle1);

Because each stream has a 1-to-1 correspondance with a libuv handle. The
code removed was never executed.

Closes neovim#980.
@aktau aktau closed this as completed in 115b165 Jul 27, 2014
fmoralesc pushed a commit to fmoralesc/neovim that referenced this issue Aug 19, 2014
Not necessary, as discussed in neovim#980.

From the libuv mailing list:
https://groups.google.com/forum/#!topic/libuv/OD38PeGeVgQ

E.g. this could happen (red: on Windows):

> > alloc_cb(handle1);
> > alloc_cb(handle2);
> > read_cb(handle1);
> > read_cb(handle2);

But this couldn't:

> > alloc_cb(handle1);
> > alloc_cb(handle1);
> > read_cb(handle1);
> > read_cb(handle1);

Because each stream has a 1-to-1 correspondance with a libuv handle. The
code removed was never executed.

Closes neovim#980.
splinterofchaos pushed a commit to splinterofchaos/neovim that referenced this issue Oct 5, 2014
splinterofchaos pushed a commit to splinterofchaos/neovim that referenced this issue Oct 5, 2014
splinterofchaos pushed a commit to splinterofchaos/neovim that referenced this issue Oct 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor changes that are not features or bugfixes
Projects
None yet
Development

No branches or pull requests

4 participants