capi: Add memory_pages_limit parameter to fizzy_instantiate #780
Conversation
gumb0
force-pushed
the
capi-memory-hard-limit
branch
from
4b3c704
to
6db3422
Compare
Codecov Report
@@ Coverage Diff @@
## master #780 +/- ##
==========================================
- Coverage 99.24% 99.21% -0.04%
==========================================
Files 79 78 -1
Lines 11964 11270 -694
==========================================
- Hits 11874 11181 -693
+ Misses 90 89 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
gumb0
force-pushed
the
capi-memory-hard-limit
branch
2 times, most recently
from
219e1c6
to
6a8321a
Compare
| @@ -410,6 +410,7 @@ bool fizzy_module_has_start_function(const FizzyModule* module) FIZZY_NOEXCEPT; | |||
| /// @param imported_globals Pointer to the imported globals array. Can be NULL iff | |||
| /// @p imported_globals_size equals 0. | |||
| /// @param imported_globals_size Size of the imported global array. Can be zero. | |||
| /// @param memory_pages_limit Hard limit for memory growth in pages. Cannot be above 65536. | |||
There was a problem hiding this comment.
Hmm, would it be a good idea allowing 0 for allowing "default setting"? Or perhaps someone actually wants to disable memory, and then 0 makes sense for that? If so, perhaps uint32_t(-1) to signal "use default"?
There was a problem hiding this comment.
I'm not sure, not a fan of special magic values.
There was a problem hiding this comment.
What is annoying is hardcoding 4096 in multiple places, like in rust.
There was a problem hiding this comment.
We could define default constant in fizzy.h, then it could be reused in tests and in rust.
There was a problem hiding this comment.
Yeah I think we should export the "default".
There was a problem hiding this comment.
I agree here. Looks good otherwise.
gumb0
force-pushed
the
capi-memory-hard-limit
branch
2 times, most recently
from
95c9ec7
to
f7d105d
Compare
test/utils/fizzy_c_engine.cpp
Outdated
| @@ -11,6 +11,8 @@ | |||
|
|
|||
| namespace fizzy::test | |||
| { | |||
| constexpr uint32_t DefaultMemoryPagesLimit = 4096; // 256MB | |||
There was a problem hiding this comment.
Why not use the one from the header?
There was a problem hiding this comment.
Forgot about this one.
| @@ -18,6 +18,7 @@ static_assert(MaxMemoryPagesLimit == 65536); | |||
|
|
|||
| /// The default hard limit of the memory size (256MB) as number of pages. | |||
| constexpr uint32_t DefaultMemoryPagesLimit = (256 * 1024 * 1024ULL) / PageSize; | |||
| static_assert(DefaultMemoryPagesLimit == 4096); | |||
There was a problem hiding this comment.
Why this is defined twice then?
There was a problem hiding this comment.
Because it doesn't worth to introduce a dependency form C++ to C API only for this yet.
There was a problem hiding this comment.
Can add a static_assert(::DefaultMemoryPagesLimit == fizzy::DefaultMemoryPagesLimit) in capi_test.cpp?
There was a problem hiding this comment.
Can add a
static_assert(::DefaultMemoryPagesLimit == fizzy::DefaultMemoryPagesLimit)incapi_test.cpp?
Added.
There was a problem hiding this comment.
Because it doesn't worth to introduce a dependency form C++ to C API only for this yet.
I don't agree here. This case returns from time to time.
gumb0
force-pushed
the
capi-memory-hard-limit
branch
2 times, most recently
from
912af4a
to
ef4d7a9
Compare
| @@ -18,6 +18,7 @@ static_assert(MaxMemoryPagesLimit == 65536); | |||
|
|
|||
| /// The default hard limit of the memory size (256MB) as number of pages. | |||
| constexpr uint32_t DefaultMemoryPagesLimit = (256 * 1024 * 1024ULL) / PageSize; | |||
| static_assert(DefaultMemoryPagesLimit == 4096); | |||
There was a problem hiding this comment.
Because it doesn't worth to introduce a dependency form C++ to C API only for this yet.
I don't agree here. This case returns from time to time.
gumb0
force-pushed
the
capi-memory-hard-limit
branch
from
ef4d7a9
to
1339c5c
Compare
No description provided.