Update tlStream.cc

[lmmir]

By reducing the time consumption caused by memmove, I tested with a 10GB GDS file and observed an 18% speed improvement. Before the modification, it took 60 seconds to read the file; after the modification, it took 43 seconds.

[klayoutmatthias]

Thanks for the PR.

First of all, when you want a PR to be accepted anywhere, please follow the style of the code base. Please do not change the indentation for example.

You have basically eliminated some memmove calls when the buffer needs to be refilled because you have increased the capacity of the buffer. I don't know whether that explains the performance improvement. GDS by nature does not need allocation of large data blocks, so the capacity stays low. Just increasing the initial capacity might have done the job too.

But in general, I do not fully understand the rationale of the code change. The goal of the function is to provide a guaranteed contiguous memory block holding the requested n bytes. The original implementation made sure that the buffer capacity would be able to hold that block by relocating the string to the beginning of the buffer and then filling the buffer with data from the file. Since the buffer is guaranteed to have at a length sufficiently large for the data block, this will also be possible. Your implementation simply appends new data to the block.

How can this work when no reallocation happens? I think that works somewhat by chance, since when there is an alignment of the data block with the buffer (blen == 0), you fill only half of the buffer and the rest remains unused. I do not think that is a guarantee. I guess there is a strong chance to trigger a buffer overflow in the general case.

Is that supposed to become a backdoor? Like the thing they tried with libxz?

Matthias

[lmmir]

Thanks for the PR.

First of all, when you want a PR to be accepted anywhere, please follow the style of the code base. Please do not change the indentation for example.

You have basically eliminated some memmove calls when the buffer needs to be refilled because you have increased the capacity of the buffer. I don't know whether that explains the performance improvement. GDS by nature does not need allocation of large data blocks, so the capacity stays low. Just increasing the initial capacity might have done the job too.

But in general, I do not fully understand the rationale of the code change. The goal of the function is to provide a guaranteed contiguous memory block holding the requested n bytes. The original implementation made sure that the buffer capacity would be able to hold that block by relocating the string to the beginning of the buffer and then filling the buffer with data from the file. Since the buffer is guaranteed to have at a length sufficiently large for the data block, this will also be possible. Your implementation simply appends new data to the block.

How can this work when no reallocation happens? I think that works somewhat by chance, since when there is an alignment of the data block with the buffer (blen == 0), you fill only half of the buffer and the rest remains unused. I do not think that is a guarantee. I guess there is a strong chance to trigger a buffer overflow in the general case.

Is that supposed to become a backdoor? Like the thing they tried with libxz?

Matthias

first “First of all, when you want a PR to be accepted anywhere, please follow the style of the code base. Please do not change the indentation for example.“ sorry about it, I will pay attention next time。

   if (mp_delegate) {
        if (m_blen == 0) {
            m_blen = mp_delegate->read(mp_buffer, m_bcap/2 );// if fill all and not half, An out-of-bounds exception 'mp_bptr + n > mp_buffer + m_bcap'  maybe occurred.
            mp_bptr = mp_buffer;
        } else {
            if (mp_bptr + n > mp_buffer + m_bcap) {
                qDebug() << "excception";
            }
            m_blen += mp_delegate->read(mp_bptr + m_blen, n - m_blen);// "n - m_blen" can ensure not use memmove next read.
        }
    }

you can try read the gds file and observe the time difference between original code and modification also.

[klayoutmatthias]

TL;DR Beware of this patch! I am not going to approve it.

I conducted some experiments and tried your patch with the following results:

1. There is no measurable performance improvement

First testcase: a 1GB OASIS file, CBLOCK compressed. With and without patch the read times are 57s.

Second testcase: a 480MB GDS file, .gz compressed. With patch, the read time is 9.7s, without patch 9.8s. That is not a significant difference. Reading the uncompressed version of this GDS file (2.4GB) gives 4.7s with patch, 4.5s without patch.

That is reading directly from a SSD on a i7-11800H @ 2.30GHz.

2. Is is very easy to trigger a buffer overflow with your implementation:

This code crashes due to memory corruption:

  tl::InputStream str ("some_larger_file");

  const char *cp = 0;
  while ((cp = str.get(10)) != 0) {
    str.unget(4);
  }

There is no way I am going to approve this PR.

I don't know if I can trust you. This patch is attacking one of the most central functions inside KLayout, the ones also used for downloading package information etc. Triggering a buffer overflow is one the methods to create security holes. I don't even know if your claims are real. I cannot see any significant runtime improvements. Maybe the numbers are made up to put me under pressure.

Matthias