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

dev.lfortran.org: use Release build #1240

Closed
Tracked by #1485
certik opened this issue Jan 31, 2023 · 7 comments · Fixed by #1494
Closed
Tracked by #1485

dev.lfortran.org: use Release build #1240

certik opened this issue Jan 31, 2023 · 7 comments · Fixed by #1494
Labels
wasm WebAssembly Backend

Comments

@certik
Copy link
Contributor

certik commented Jan 31, 2023

Currently it is using Debug build.

@certik certik added the wasm WebAssembly Backend label Jan 31, 2023
@Shaikh-Ubaid
Copy link
Member

Shaikh-Ubaid commented Mar 25, 2023

For release build, I am using the following changes:

diff --git a/build_to_wasm.sh b/build_to_wasm.sh
index 382663dc3..e2d74f95d 100755
--- a/build_to_wasm.sh
+++ b/build_to_wasm.sh
@@ -3,7 +3,7 @@
 set -ex
 
 cmake \
-    -DCMAKE_BUILD_TYPE=Debug \
+    -DCMAKE_BUILD_TYPE=Release \
     -DWITH_LLVM=yes \
     -DLFORTRAN_BUILD_ALL=yes \
     -DWITH_STACKTRACE=no \
@@ -17,7 +17,7 @@ cp src/runtime/*.mod src/bin/asset_dir
 git clean -dfx -e src/bin/asset_dir
 
 emcmake cmake \
-    -DCMAKE_BUILD_TYPE=Debug \
+    -DCMAKE_BUILD_TYPE=Release \
     -DCMAKE_CXX_FLAGS_DEBUG="-Wall -Wextra -fexceptions" \
     -DWITH_LLVM=no \
     -DLFORTRAN_BUILD_ALL=yes \
diff --git a/src/lfortran/parser/parser_stype.h b/src/lfortran/parser/parser_stype.h
index c197c77bf..009557569 100644
--- a/src/lfortran/parser/parser_stype.h
+++ b/src/lfortran/parser/parser_stype.h
@@ -93,7 +93,7 @@ static_assert(std::is_trivial<YYSTYPE>::value);
 // Ensure the YYSTYPE size is equal to Vec<AST::ast_t*>, which is a required member, so
 // YYSTYPE has to be at least as big, but it should not be bigger, otherwise it
 // would reduce performance.
-static_assert(sizeof(YYSTYPE) == sizeof(Vec<AST::ast_t*>));
+// static_assert(sizeof(YYSTYPE) == sizeof(Vec<AST::ast_t*>));
 
 } // namespace LCompilers::LFortran

(the static_assert(sizeof(YYSTYPE) == sizeof(Vec<AST::ast_t*>)); fails for release build. I think this was previously noted and it was shared that we can comment it out.)

With the release build, it throws a bad_alloc exception:
image

(I think it is similar to the one discussed at https://lfortran.zulipchat.com/#narrow/stream/197339-general/topic/Building.20LFortran.20in.20emscripten-forge/near/292549746 and #585)

The exact issue comes when we are allocating/reserving space for wasm section streams. It especially fails when reserving space for the last stream m_data_section.

m_type_section.reserve(m_al, 1024 * 128);
m_import_section.reserve(m_al, 1024 * 128);
m_func_section.reserve(m_al, 1024 * 128);
m_memory_section.reserve(m_al, 1024 * 128);
m_global_section.reserve(m_al, 1024 * 128);
m_export_section.reserve(m_al, 1024 * 128);
m_code_section.reserve(m_al, 1024 * 128);
m_data_section.reserve(m_al, 1024 * 128);

Surprisingly, the following diff seems to get it working (I am currently unsure why/how it works):

diff --git a/src/libasr/alloc.h b/src/libasr/alloc.h
index 76588a288..81902ab0f 100644
--- a/src/libasr/alloc.h
+++ b/src/libasr/alloc.h
@@ -6,6 +6,7 @@
 #include <new>
 #include <stdexcept>
 #include <vector>
+#include <iostream>
 
 #include <libasr/assert.h>
 
@@ -52,6 +53,7 @@ public:
         // force new_chunk() not to get inlined, but there is no standard way of
         // doing it. This try/catch approach effectively achieves the same using
         // standard C++.
+        std::cout << std::endl;
         try {
             LCOMPILERS_ASSERT(start != nullptr);
             size_t addr = current_pos;

(we can also write any string inside the above std::cout)

image

@certik
Copy link
Contributor Author

certik commented Mar 25, 2023

Thanks for debugging it. We'll have to figure out what is causing it.

@certik
Copy link
Contributor Author

certik commented Mar 28, 2023

Here is how we can fix this:

--- a/src/libasr/alloc.h
+++ b/src/libasr/alloc.h
@@ -52,15 +52,25 @@ public:
         // force new_chunk() not to get inlined, but there is no standard way of
         // doing it. This try/catch approach effectively achieves the same using
         // standard C++.
+#ifndef LCOMPILERS_FAST_ALLOC
         try {
+#endif
             LCOMPILERS_ASSERT(start != nullptr);
             size_t addr = current_pos;
             current_pos += align(s);
-            if (size_current() > size_total()) throw std::bad_alloc();
+            if (size_current() > size_total()) {
+#ifdef LCOMPILERS_FAST_ALLOC
+                throw std::bad_alloc();
+#else
+                return new_chunk(s);
+#endif
+            }
             return (void*)addr;
+#ifndef LCOMPILERS_FAST_ALLOC
         } catch (const std::bad_alloc &e) {
             return new_chunk(s);
         }
+#endif
     }
 
     void *new_chunk(size_t s) {

And have LCOMPILERS_FAST_ALLOC=ON in cmake by default, but in Release mode for Emscripten you can set it to off.

@Shaikh-Ubaid
Copy link
Member

Shaikh-Ubaid commented Mar 30, 2023

This #1240 (comment) seems to work. I am slightly concerned if it is a robust solution. I will see if there are better approaches to it.

@Shaikh-Ubaid
Copy link
Member

A concern here was that exceptions are not being supported in release mode as shared by you. I found the fix. I will be submitting a PR soon.

@Shaikh-Ubaid
Copy link
Member

Shaikh-Ubaid commented Apr 4, 2023

The size for generated lfortran.wasm in release mode is around ~5mb which seems longer.

PS: I just noticed that the size for lfortran.wasm in debug mode is also around ~5mb. I remember it used to be around ~2.5 mbs a few months back. It seems that it almost doubled.

@certik
Copy link
Contributor Author

certik commented Apr 5, 2023

Yeah, and we might need to start removing backends (via cmake options) and features that we don't need for the WASM application, to keep the size low.

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

Successfully merging a pull request may close this issue.

2 participants