Re: [cmake-developers] [PATCH] Avoid bad alloc for large files
On 12/23/2014 4:27 AM, Rolf Eike Beer wrote: > cmCTest.cxx > C:\Dashboards\My Tests\CMakeNext-vs12-64-ninja-src\Source\cmCTest.cxx(1705) : > warning C4267: 'argument' : conversion from 'size_t' to 'unsigned long', > possible loss of data > > The problem is: this is absolutely right. On Windows64 this means that the > maximum file size will be 4GB, or one needs to change the base64 > implementation. Which way to go? The KWSys Base64 API should be fixed to use size_t: http://review.source.kitware.com/18579 Domen's suggestion elsewhere in this thread of using a streaming implementation will be necessary to support large files on 32-bit platforms. That can be done as follow-up work though. -Brad -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] [PATCH] Avoid bad alloc for large files
Am Dienstag, 23. Dezember 2014, 10:27:47 schrieb Rolf Eike Beer: > Am Montag, 22. Dezember 2014, 22:16:50 schrieb Rolf Eike Beer: > > Am Montag, 22. Dezember 2014, 10:50:28 schrieb Brad King: > > > On 12/20/2014 08:44 AM, Rolf Eike Beer wrote: > > > > This is basically the same, but it avoids the needless floating point > > > > arithmetic. Does it work for you? > > > > > > Thanks, Eike. Please add a topic to put this in 'next' when ready. > > > > Done. I wonder if we should not check for new returning null? > > Ok, now I get this: > > cmCTest.cxx > C:\Dashboards\My Tests\CMakeNext-vs12-64-ninja-src\Source\cmCTest.cxx(1705) > : warning C4267: 'argument' : conversion from 'size_t' to 'unsigned long', > possible loss of data > > The problem is: this is absolutely right. On Windows64 this means that the > maximum file size will be 4GB, or one needs to change the base64 > implementation. Which way to go? *lol* From: "Brad King (Code Review)" Date: Tue, 23 Dec 2014 04:27:55 -0500 Subject: Change in KWSys[master]: Base64: Use size_t for lenghts in API signature.asc Description: This is a digitally signed message part. -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] [PATCH] Avoid bad alloc for large files
Am Montag, 22. Dezember 2014, 22:16:50 schrieb Rolf Eike Beer: > Am Montag, 22. Dezember 2014, 10:50:28 schrieb Brad King: > > On 12/20/2014 08:44 AM, Rolf Eike Beer wrote: > > > This is basically the same, but it avoids the needless floating point > > > arithmetic. Does it work for you? > > > > Thanks, Eike. Please add a topic to put this in 'next' when ready. > > Done. I wonder if we should not check for new returning null? Ok, now I get this: cmCTest.cxx C:\Dashboards\My Tests\CMakeNext-vs12-64-ninja-src\Source\cmCTest.cxx(1705) : warning C4267: 'argument' : conversion from 'size_t' to 'unsigned long', possible loss of data The problem is: this is absolutely right. On Windows64 this means that the maximum file size will be 4GB, or one needs to change the base64 implementation. Which way to go? Eike signature.asc Description: This is a digitally signed message part. -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] [PATCH] Avoid bad alloc for large files
Am Montag, 22. Dezember 2014, 10:50:28 schrieb Brad King: > On 12/20/2014 08:44 AM, Rolf Eike Beer wrote: > > This is basically the same, but it avoids the needless floating point > > arithmetic. Does it work for you? > > Thanks, Eike. Please add a topic to put this in 'next' when ready. Done. I wonder if we should not check for new returning null? Eike signature.asc Description: This is a digitally signed message part. -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] [PATCH] Avoid bad alloc for large files
>>> I received a bad alloc when uploading a large file with CTest. The patch >>> below resolved this. I just took a look at the code and noticed that it is quite memory consumption heavy ((2 * encoded_buffer_size) + file_buffer_size). This implementation could be used instead (not tested): std::string cmCTest::Base64EncodeFile(const std::string& file) { const size_t len = cmSystemTools::FileLength(file); cmsys::ifstream ifs(file.c_str(), std::ios::in #ifdef _WIN32 | std::ios::binary #endif ); std::string base64; // set to empty string by default // section for throwing encoded_buffer out of scope as soon as possible to // use up less memory { const size_t output_size = ((len - 1) / 3) * 4 + 4; const size_t final_size = output_size + (output_size / 64) * 2; std::vector encoded_buffer; encoded_buffer.resize(final_size); // section for throwing file_buffer out of scope as soon as possible to // use up less memory { std::vector file_buffer; file_buffer.resize(len); ifs.read(reinterpret_cast(file_buffer), len); ifs.close(); unsigned long rlen = cmsysBase64_Encode(&file_buffer[0], len, &encoded_buffer[0], 1); } // consume less memory by swapping tmp string buffer with base64 variable std::string(encoded_buffer.begin(), encoded_buffer.begin() + rlen).swap(base64); } return base64; // this will produce another copy in C++98 } It lowers memory consumption to approximately (2 * encoded_buffer_size) and it also makes memory deallocation exception safe (file_buffer and encoded_buffer variables). A better solution would be to rewrite the function to: void cmCTest::Base64EncodeFile(const std::string& file, std::string& base64) to prevent one more copy at return but since this changes the interface we could go a step further and simply rewrite the entire function and cmsysBase64_Encode function to use streams and thereby lower the memory consumption even further (less copying between different containers and more readable than the above version). Thoughts? p.s. I think that using std::ios::binary would work on all platforms so I guess that the function does not need ifdef at the top. Regards, Domen -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] [PATCH] Avoid bad alloc for large files
>> I received a bad alloc when uploading a large file with CTest. The patch >> below resolved this. > > Your patch is line-wrapped and can't be applied. However, I did this by hand. > This is basically the same, but it avoids the needless floating point > arithmetic. Does it work for you? snip > std::string cmCTest::Base64EncodeFile(std::string file) snip > -static_cast(len) * 1.5 + 5.0) ]; > += new unsigned char [ (len * 3) / 2 + 5 ]; snip I came across a similar issue a few days ago in our code base (bad alloc when compressing a large file in-memory with 256 MB data ulimit per process) and I used a different formula to calculate the maximum buffer size: http://stackoverflow.com/questions/1533113/calculate-the-size-to-a-base-64-encoded-message I used a bit modified answer from "kanaka". size_t output_size = ((len - 1) / 3) * 4 + 4; size_t final_size = output_size + (output_size / 64) * 2; // 64 instead of 76 since RFC 3548 and RFC 4648 allow CLRF line breaks every 64 characters This formula would give less memory allocation overhead. Regards, Domen -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] [PATCH] Avoid bad alloc for large files
On 12/20/2014 08:44 AM, Rolf Eike Beer wrote: > This is basically the same, but it avoids the needless floating point > arithmetic. Does it work for you? Thanks, Eike. Please add a topic to put this in 'next' when ready. Thanks, -Brad -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] [PATCH] Avoid bad alloc for large files
Yes, thank you. On Saturday, December 20, 2014, Rolf Eike Beer wrote: > Justin Borodinsky wrote: > > I received a bad alloc when uploading a large file with CTest. The patch > > below resolved this. > > Your patch is line-wrapped and can't be applied. However, I did this by > hand. > This is basically the same, but it avoids the needless floating point > arithmetic. Does it work for you? > > diff --git a/Source/cmCTest.cxx b/Source/cmCTest.cxx > index 2bf7b77..1a7bf45 100644 > --- a/Source/cmCTest.cxx > +++ b/Source/cmCTest.cxx > @@ -1688,7 +1688,7 @@ std::string > cmCTest::Base64GzipEncodeFile(std::string file) > //-- > std::string cmCTest::Base64EncodeFile(std::string file) > { > - long len = cmSystemTools::FileLength(file); > + const size_t len = cmSystemTools::FileLength(file); >cmsys::ifstream ifs(file.c_str(), std::ios::in > #ifdef _WIN32 > | std::ios::binary > @@ -1699,8 +1699,7 @@ std::string cmCTest::Base64EncodeFile(std::string > file) >ifs.close(); > >unsigned char *encoded_buffer > -= new unsigned char [ static_cast( > -static_cast(len) * 1.5 + 5.0) ]; > += new unsigned char [ (len * 3) / 2 + 5 ]; > >unsigned long rlen > = cmsysBase64_Encode(file_buffer, len, encoded_buffer, 1); > > > > Eike > -- -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] [PATCH] Avoid bad alloc for large files
Justin Borodinsky wrote: > I received a bad alloc when uploading a large file with CTest. The patch > below resolved this. Your patch is line-wrapped and can't be applied. However, I did this by hand. This is basically the same, but it avoids the needless floating point arithmetic. Does it work for you? diff --git a/Source/cmCTest.cxx b/Source/cmCTest.cxx index 2bf7b77..1a7bf45 100644 --- a/Source/cmCTest.cxx +++ b/Source/cmCTest.cxx @@ -1688,7 +1688,7 @@ std::string cmCTest::Base64GzipEncodeFile(std::string file) //-- std::string cmCTest::Base64EncodeFile(std::string file) { - long len = cmSystemTools::FileLength(file); + const size_t len = cmSystemTools::FileLength(file); cmsys::ifstream ifs(file.c_str(), std::ios::in #ifdef _WIN32 | std::ios::binary @@ -1699,8 +1699,7 @@ std::string cmCTest::Base64EncodeFile(std::string file) ifs.close(); unsigned char *encoded_buffer -= new unsigned char [ static_cast( -static_cast(len) * 1.5 + 5.0) ]; += new unsigned char [ (len * 3) / 2 + 5 ]; unsigned long rlen = cmsysBase64_Encode(file_buffer, len, encoded_buffer, 1); Eike -- signature.asc Description: This is a digitally signed message part. -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
[cmake-developers] [PATCH] Avoid bad alloc for large files
I received a bad alloc when uploading a large file with CTest. The patch below resolved this. --- Source/cmCTest.cxx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Source/cmCTest.cxx b/Source/cmCTest.cxx index 2bf7b77..52bf651 100644 --- a/Source/cmCTest.cxx +++ b/Source/cmCTest.cxx @@ -1688,7 +1688,7 @@ std::string cmCTest::Base64GzipEncodeFile(std::string file) //-- std::string cmCTest::Base64EncodeFile(std::string file) { - long len = cmSystemTools::FileLength(file); + std::size_t len = static_cast(cmSystemTools::FileLength(file)); cmsys::ifstream ifs(file.c_str(), std::ios::in #ifdef _WIN32 | std::ios::binary @@ -1699,7 +1699,7 @@ std::string cmCTest::Base64EncodeFile(std::string file) ifs.close(); unsigned char *encoded_buffer -= new unsigned char [ static_cast( += new unsigned char [ static_cast( static_cast(len) * 1.5 + 5.0) ]; unsigned long rlen -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers