Make hard memory limit configurable #515

gumb0 · 2020-09-02T11:40:08Z

Closes #509

codecov · 2020-09-02T14:01:33Z

Codecov Report

Merging #515 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master     #515    +/-   ##
========================================
  Coverage   99.69%   99.69%            
========================================
  Files          54       54            
  Lines       17153    17322   +169     
========================================
+ Hits        17101    17270   +169     
  Misses         52       52

axic · 2020-09-02T16:30:26Z

lib/fizzy/execute.cpp

@@ -1229,9 +1230,10 @@ ExecutionResult execute(
            uint32_t ret = static_cast<uint32_t>(cur_pages);
            try
            {
+                // TODO use memory_pages_limit passed to instantiate
                const size_t memory_max_pages =


Couldn't we update instance.memory_pages_limit with the imported max? So execute would be only concerned with instance.memory_pages_limit.

I'll try it

axic · 2020-09-02T16:33:02Z

Looks good in general, this is what I meant.

axic · 2020-09-02T17:47:42Z

lib/fizzy/execute.cpp

-    auto [memory, memory_limits] = allocate_memory(module.memorysec, imported_memories);
+    auto [memory, memory_limits] =
+        allocate_memory(module.memorysec, imported_memories, memory_pages_limit);
+    if (memory_limits.max.has_value())


Would add something like:

Suggested change

if (memory_limits.max.has_value())

// If imported memory is used, we need to adjust the memory limit.

// Note: allocate_memory ensures this limit is always below memory_pages_limit.

if (memory_limits.max.has_value())

I added the comment, but it applies to non-imported memory, too.

Ah right, it is the memory section setting.

axic · 2020-09-02T17:48:45Z

test/spectests/spectests.cpp

+                    m_instances[name] =
+                        fizzy::instantiate(std::move(module), std::move(imports.functions),
+                            std::move(imports.tables), std::move(imports.memories),
+                            std::move(imports.globals), TestMemoryPagesLimit);


I'll actually override the default in #329 too.

chfast · 2020-09-03T12:05:10Z

lib/fizzy/execute.cpp

@@ -173,7 +172,7 @@ std::tuple<table_ptr, Limits> allocate_table(
 }

 std::tuple<bytes_ptr, Limits> allocate_memory(const std::vector<Memory>& module_memories,
-    const std::vector<ExternalMemory>& imported_memories)
+    const std::vector<ExternalMemory>& imported_memories, unsigned memory_pages_limit)


Suggested change

const std::vector<ExternalMemory>& imported_memories, unsigned memory_pages_limit)

const std::vector<ExternalMemory>& imported_memories, uint32_t memory_pages_limit)

Always use uint32_t for memory indexing and sizes?

This is number of pages, the limits sections uses uint32_t, and we actually use size_t in multiple places regarding this.

Yeah I made it unsigned because constants in limits.hpp are already unsigned, and also because it's a limit, that we impose ourselves, not related to the sizes in the spec... But now I think it makes sense to make everything uint32_t

chfast · 2020-09-03T12:06:21Z

lib/fizzy/execute.cpp

@@ -677,7 +676,8 @@ std::optional<uint32_t> find_export(const Module& module, ExternalKind kind, std

 std::unique_ptr<Instance> instantiate(Module module,
    std::vector<ExternalFunction> imported_functions, std::vector<ExternalTable> imported_tables,
-    std::vector<ExternalMemory> imported_memories, std::vector<ExternalGlobal> imported_globals)
+    std::vector<ExternalMemory> imported_memories, std::vector<ExternalGlobal> imported_globals,
+    unsigned memory_pages_limit /* = DefaultMemoryPagesLimit */)


Suggested change

unsigned memory_pages_limit /* = DefaultMemoryPagesLimit */)

unsigned memory_pages_limit /*= DefaultMemoryPagesLimit*/)

To be consistent with /*unused*/?

chfast · 2020-09-03T12:11:58Z

lib/fizzy/limits.hpp

@@ -14,8 +14,8 @@ constexpr unsigned PageSize = 65536;
 constexpr unsigned MemoryPagesValidationLimit = (4 * 1024 * 1024 * 1024ULL) / PageSize;
 static_assert(MemoryPagesValidationLimit == 65536);

-// Set hard limit of 256MB of memory.
-constexpr unsigned MemoryPagesLimit = (256 * 1024 * 1024ULL) / PageSize;
+// Set default hard limit of 256MB of memory.


Suggested change

// Set default hard limit of 256MB of memory.

// The default hard limit of the memory size (256MB) as number of pages.

Also use uint32_t for memory size limits.

gumb0 force-pushed the configure-mem-limit branch from 7f7c53b to 84a2e27 Compare September 2, 2020 12:19

axic reviewed Sep 2, 2020

View reviewed changes

gumb0 force-pushed the configure-mem-limit branch 2 times, most recently from 24616c2 to 8b3c2b8 Compare September 2, 2020 17:18

axic reviewed Sep 2, 2020

View reviewed changes

gumb0 force-pushed the configure-mem-limit branch from 9295c81 to ca23b2f Compare September 3, 2020 11:47

gumb0 marked this pull request as ready for review September 3, 2020 11:48

gumb0 requested review from chfast and axic September 3, 2020 12:00

chfast reviewed Sep 3, 2020

View reviewed changes

gumb0 force-pushed the configure-mem-limit branch from ca23b2f to 6b39f2b Compare September 3, 2020 12:53

chfast approved these changes Sep 3, 2020

View reviewed changes

axic approved these changes Sep 3, 2020

View reviewed changes

gumb0 added 4 commits September 4, 2020 11:33

Configure hard memory limit in instantiate

75e629a

Also use uint32_t for memory size limits.

test: Set hard memory limit to 4 Gb in spectests runner

7ac48ee

ci: update spectest expectations

a8cb9b8

test: custom memory hard limit

696a9f2

gumb0 force-pushed the configure-mem-limit branch from 6b39f2b to 696a9f2 Compare September 4, 2020 09:33

gumb0 merged commit 2332409 into master Sep 4, 2020

gumb0 deleted the configure-mem-limit branch September 4, 2020 09:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make hard memory limit configurable #515

Make hard memory limit configurable #515

gumb0 commented Sep 2, 2020

codecov bot commented Sep 2, 2020 •

edited

Loading

axic Sep 2, 2020 •

edited

Loading

gumb0 Sep 2, 2020

axic commented Sep 2, 2020

axic Sep 2, 2020

gumb0 Sep 3, 2020

axic Sep 3, 2020

axic Sep 2, 2020

chfast Sep 3, 2020

axic Sep 3, 2020

gumb0 Sep 3, 2020

chfast Sep 3, 2020

chfast Sep 3, 2020

	const std::vector<ExternalMemory>& imported_memories, unsigned memory_pages_limit)
	const std::vector<ExternalMemory>& imported_memories, uint32_t memory_pages_limit)

	unsigned memory_pages_limit /* = DefaultMemoryPagesLimit */)
	unsigned memory_pages_limit /= DefaultMemoryPagesLimit/)

	// Set default hard limit of 256MB of memory.
	// The default hard limit of the memory size (256MB) as number of pages.

Make hard memory limit configurable #515

Make hard memory limit configurable #515

Conversation

gumb0 commented Sep 2, 2020

codecov bot commented Sep 2, 2020 • edited Loading

Codecov Report

axic Sep 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Sep 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 2, 2020 •

edited

Loading

axic Sep 2, 2020 •

edited

Loading