Skip to content

Conversation

@shinrich
Copy link
Member

Using @bryancall's proxy.config.res_track_memory feature to figure out buffer allocations, it showed that the buffer set up by HttpSM::setup_server_transfer had over 2000 allocations for a H2 transfer of 10MB but only 600 allocations for the same transfer over H1.

This code change, reduced the number of allocations to around 1000 for H2. Still higher than H1, but much better.

The write_avail will never return 0 and always allocate a new block if the current writer block is full. So the if return should never trigger. In the H2 logic, we are copying IOBlocks from the origin side MIOBuffer, so the empty block will never be used. Could explain why we started seeing unusually long buffer block chains.

@shinrich shinrich added this to the 10.0.0 milestone Aug 30, 2019
@shinrich shinrich requested a review from masaori335 August 30, 2019 15:08
@shinrich shinrich self-assigned this Aug 30, 2019
@masaori335
Copy link
Contributor

MIOBuffer::write_avail() is tricky especially when water_mark is set. I agree with some IOBlock allocations are wasted.

The write_avail will never return 0 and always allocate a new block if the current writer block is full. So the if return should never trigger.

From my understanding, this is not accurate.

  • the block size & warter_mark of this MIOBuffer are 32KB (assuming default config)
  • when the current writer block is full (max_read_avail() returns more than 32KB)
  • high_water() returns 1 and current_write_avail() returns 0
  • check_add_block() do nothing
  • write_avail() returns 0

This scenario could happen when Http2Stream is not the only consumer of HttpTunnel (e.g. "cache write").

Copy link
Contributor

@masaori335 masaori335 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need more investigation about MIOBuffer::write_avail() with water_mark.

", reader.read_avail=%" PRId64,
write_vio.nbytes, write_vio.ndone, write_vio.get_writer()->write_avail(), bytes_avail);
Http2StreamDebug("write_vio.nbytes=%" PRId64 ", write_vio.ndone=%" PRId64 ", reader.read_avail=%" PRId64, write_vio.nbytes,
write_vio.ndone, bytes_avail);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is definitely good. We should not call functions in Debug that have a side effect.

@zwoop
Copy link
Contributor

zwoop commented Oct 17, 2019

@vmamidi Can you look at this please.

@bryancall bryancall requested a review from vmamidi October 17, 2019 18:19
@zwoop zwoop added the OnDocs This is for PR currently running, or will run, on the Docs ATS server label Mar 11, 2020
@shinrich
Copy link
Member Author

I think PR #5903 has address this issue. Closing.

@shinrich shinrich closed this Jul 23, 2020
@shinrich shinrich removed this from the 10.0.0 milestone Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

HTTP/2 OnDocs This is for PR currently running, or will run, on the Docs ATS server

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants