Add special build for testing serialization via a serialization roundtrip in JIT compilation and fix serialization leaks #7763

TH3CHARLie · 2023-08-15T21:14:12Z

relates #7745

As discussed in dev meeting, I add back the JIT roundtrip test. It works by intercepting the compile_jit call, doing a serialization roundtrip inside it and using serialized results to do the compilation.

Because its rather hacky, the code is included in #ifdef blocks, and a compile option WITH_SERIALIZATION_JIT is added, this should be a test-only option that builds a special version of Halide that have no use other than serialization testing. Under this special build, correctness test suite becomes serialization correctness test.

Welcome all thoughts of comments and feedbacks @steven-johnson @abadams @derek-gerstmann

src/CMakeLists.txt

steven-johnson · 2023-08-16T18:32:20Z

src/Pipeline.cpp

@@ -582,6 +582,25 @@ void Pipeline::compile_jit(const Target &target_arg) {
    // Clear all cached info in case there is an error.
    contents->invalidate_cache();

+#ifdef WITH_SERIALZATION_JIT
+    // TODO: replace file serialization with in-memory serialization


Nit: please open a Issue for this task and add the comment here e.g. TODO(http://etc):

Derek already implement the API in his PR so I will refer to that

src/Pipeline.cpp

steven-johnson

LGTM with minor nits

TH3CHARLie · 2023-08-16T18:54:12Z

Updated

steven-johnson · 2023-08-22T22:59:09Z

src/CMakeLists.txt

@@ -508,6 +508,15 @@ if (WITH_SERIALIZATION)
    target_compile_definitions(Halide PRIVATE WITH_SERIALIZATION)
 endif ()

+# Enable serialization testing by intercepting JIT compilation with a serialization roundtrip;
+# This is used only for special builds made specifically for testing, and must be disabled by default.
+option(WITH_SERIALIZATION_JIT "Intercepting JIT compilation with a serialization roundtrip, for test only" OFF)


Suggestion: rename this to something more evocative of testing, e.g. WITH_SERIALIZATION_JIT_ROUNDTRIP_TESTING

Update: I took the liberty of making this change myself, so that I could test the necessary buildbot changes

sry, I should have done it last night, thanks!

steven-johnson · 2023-08-24T00:22:36Z

(I'll see if I can get a local build with symbols enabled to get any better info on this)

steven-johnson

We have to understand how/why these leaks are occurring before this can land -- are they intrinsic to the serializer, or a byproduct of the testing hack?

TH3CHARLie · 2023-08-24T00:36:22Z

thanks for reporting this, this directly confirms my previous hypothesis that serialization code contains leaks, we found it because our fuzzer driver will oom after running 30k ish pipelines(each run has a deserialization call). i'm pretty sure it's not because of the jit hack, it's part of the serialization. Last time i had no luck using valgrind, tmr i'll try figure it out

derek-gerstmann · 2023-08-24T00:41:43Z

It’s probably easier/more accurate to use the asan build configuration for Halide. Val grind is notoriously slow since it’s a runtime intercept. If you look at the symbolized call sites that Steven posted, the allocation locations should lead you to areas to investigate. It appears to be IR and Parameter references that aren’t getting cleaned up or have ownership cycles.

abadams · 2023-08-24T01:18:11Z

One place to check is any new constructor-like things you have added, e.g. Function::update_with_deserialization

The normal ways Functions acquire right-hand-sides have various checks in place to avoid reference cycles, and that may not be true of the new ways you added.

TH3CHARLie · 2023-08-25T20:01:58Z

One place to check is any new constructor-like things you have added, e.g. Function::update_with_deserialization

The normal ways Functions acquire right-hand-sides have various checks in place to avoid reference cycles, and that may not be true of the new ways you added.

Can you give an example of the existing checks?

TH3CHARLie · 2023-08-25T23:20:56Z

okay, it's just like Andrew has guessed, Function::update_with_deserialization is the root cause of all leaks, why? because in build_reverse_function_mapping I added the function ptr as they are and they are later used in places like wrappers and extern func args, while all these function ptrs are strong ptrs. I have a fix now works on some cases I've tested, will file a PR once all tests passed locally.

TH3CHARLie · 2023-08-25T23:43:54Z

Maybe pushing the updates here in this PR is easier for testing?

steven-johnson · 2023-09-26T19:15:07Z

OK, fix applied, wait for it to go green (again)

steven-johnson · 2023-09-26T23:03:05Z

Still one failure:

https://buildbot.halide-lang.org/master/#/builders/42/builds/54/steps/19/logs/correctness_vector_math

/home/halidenightly/build_bot/worker/halide-testbranch-main-llvm18-x86-64-linux-cmake/halide-build/test/correctness/correctness_vector_math
vector_math test seed: 1695764725
Testing floatx4
Testing floatx8
Testing doublex2
Testing uint8_tx16
Testing int8_tx16
Testing uint16_tx8
Warning: deserialized pipeline is empty
Required regular expression not found. Regex=[Success!]

TH3CHARLie · 2023-09-27T07:16:52Z

Locally this test passed, I have a theory that we are deserializing from files and several tests are running together with some race issue on the same file names (i.e. f0.hlpipe), that's why we are seeing deserializing from empty pipeline warnings.

…testing

TH3CHARLie · 2023-09-28T21:53:10Z

To verify my theory, the new commit now uses serialize to memory instead of files, passed all tests on CPU locally, let's see how it goes on the buildbots.

TH3CHARLie · 2023-09-29T18:25:32Z

These failures shouldn't be related to this PR.....

abadams · 2023-09-29T18:44:43Z

Looks like the serialization tutorial is throwing an error

TH3CHARLie · 2023-09-29T18:49:33Z

I see the same error in the tutorial PR, then what about the fast_pow one https://buildbot.halide-lang.org/master/#/builders/82/builds/75

derek-gerstmann · 2023-09-29T19:00:05Z

I see the same error in the tutorial PR, then what about the fast_pow one https://buildbot.halide-lang.org/master/#/builders/82/builds/75

I'm confused ... I don't see a failure for the serialization tutorial in the PR I made: #7760

Oh ... wait ... nvm. Sorry ... it appears to fail due to WITH_SERIALIZATION not being set on OSX?

derek-gerstmann · 2023-09-29T19:14:34Z

@TH3CHARLie Sorry for not catching this .... adding this to the Makefile should resolve the tutorial issue:


Makefile Line 286:
...
TUTORIAL_CXX_FLAGS ?= -std=c++17 -g -fno-omit-frame-pointer $(RTTI_CXX_FLAGS) -I $(ROOT_DIR)/tools $(SANITIZER_FLAGS) $(LLVM_CXX_FLAGS_LIBCPP)
+ ifneq (,$(shell which flatc))
+ TUTORIAL_CXX_FLAGS += -DWITH_SERIALIZATION -I $(BUILD_DIR) -I $(shell which flatc | sed 's/bin.flatc/include/')
+ endif
...

TH3CHARLie · 2023-10-02T17:14:51Z

@derek-gerstmann tutorial make failure still exsits, the x86 failure seems unrelated though

derek-gerstmann · 2023-10-02T19:23:55Z

@derek-gerstmann tutorial make failure still exsits, the x86 failure seems unrelated though

That would indicate that flatc isn't installed on the machine. You can add a rule to skip running the tutorial if flatc is not found (right after the

tutorial_%: $(BIN_DIR)/tutorial_% $(TMP_DIR)/images/rgb.png $(TMP_DIR)/images/gray.png
	@-mkdir -p $(TMP_DIR)
	cd $(TMP_DIR) ; $(CURDIR)/$<
	@-echo

+ # Skip the serialization tutorial, if we didn't build -DWITH_SERIALIZATION
+ ifeq (,$(shell which flatc))
+ tutorial_lesson_23_serialization: 
+ 	@echo "Skipping tutorial lesson 23 (serialization not enabled) ..." 
+ endif

derek-gerstmann · 2023-10-02T19:25:01Z

Also, it looks like the makefile checks for flatc ... shouldn't it be checking for flatcc?

TH3CHARLie · 2023-10-03T19:13:00Z

OK now all failures look unrelated (failing on host targets that are not serialization test), PTAL @steven-johnson @derek-gerstmann @abadams

abadams · 2023-10-05T17:24:49Z

Makefile

@@ -288,6 +288,9 @@ LLVM_SHARED_LIBS = -Wl,-rpath=$(LLVM_LIBDIR) -L $(LLVM_LIBDIR) -lLLVM
 LLVM_LIBS_FOR_SHARED_LIBHALIDE=$(if $(WITH_LLVM_INSIDE_SHARED_LIBHALIDE),$(LLVM_STATIC_LIBS),$(LLVM_SHARED_LIBS))

 TUTORIAL_CXX_FLAGS ?= -std=c++17 -g -fno-omit-frame-pointer $(RTTI_CXX_FLAGS) -I $(ROOT_DIR)/tools $(SANITIZER_FLAGS) $(LLVM_CXX_FLAGS_LIBCPP)
+ifneq (,$(shell which flatc))
+TUTORIAL_CXX_FLAGS += -DWITH_SERIALIZATION -I $(BUILD_DIR) -I $(shell which flatc | sed 's/bin.flatc/include/')


Are you sure this is necessary? These flags are the ones that would be set by users of Halide. It would be bad if they needed to set any of this to use serialization.

I think we should just remove this part, we already skipping tutorial 23 if no flatc found.

src/Serialization.cpp

TH3CHARLie · 2023-11-06T23:36:46Z

Most failures are related to webGPU, bad_partition_always is failing due to a missing serialization, which I will fix in a later PR, landing this now.

…trip in JIT compilation and fix serialization leaks (halide#7763) * add back JIT testing, enclosed in #ifdef blocks * fix typo * nits * WITH_SERIALIZATION_JIT->WITH_SERIALIZATION_JIT_ROUNDTRIP_TESTING * fix self-reference leaks: now uses weak function ptr in reverse function mappings * Move clang-tidy checks back to Linux Recent changes in the GHA runners for macOS don't play well with clang-tidy; rather than sink any more time into debugging it, I'm going to revert the relevant parts of halide#7746 so that it runs on the less-finicky Linux runners instead. * bogus * Update Generator.cpp * Update Generator.cpp * call copy_to_host before serializing buffers * throw an error if we serialize on-device buffer * Skip specialize_to_gpu * Update Pipeline.cpp * Skip two more tests * use serialize to memory during jit testing * makefile update * makefile fix * skip the tutorial if flatc is not there * fix * fix signature * fix makefile * trigger buildbot --------- Co-authored-by: Steven Johnson <[email protected]>

TH3CHARLie added 2 commits August 15, 2023 14:02

add back JIT testing, enclosed in #ifdef blocks

c0cf44d

fix typo

d674662

TH3CHARLie marked this pull request as draft August 15, 2023 21:14

TH3CHARLie changed the title ~~Add special build for testing serialization via JIT roundtrip~~ Add special build for testing serialization via a serialization roundtrip in JIT compilation Aug 15, 2023

TH3CHARLie marked this pull request as ready for review August 15, 2023 22:08

steven-johnson reviewed Aug 16, 2023

View reviewed changes

src/CMakeLists.txt Outdated Show resolved Hide resolved

steven-johnson reviewed Aug 16, 2023

View reviewed changes

src/Pipeline.cpp Outdated Show resolved Hide resolved

steven-johnson reviewed Aug 16, 2023

View reviewed changes

src/Pipeline.cpp Outdated Show resolved Hide resolved

steven-johnson requested changes Aug 16, 2023

View reviewed changes

nits

3a2de83

steven-johnson approved these changes Aug 16, 2023

View reviewed changes

steven-johnson requested a review from halidebuildbots August 16, 2023 20:07

steven-johnson reviewed Aug 22, 2023

View reviewed changes

steven-johnson added a commit to halide/build_bot that referenced this pull request Aug 22, 2023

Modify buildbots to enable testing for halide/Halide#7763

7bb4a7b

steven-johnson added 3 commits August 23, 2023 09:32

WITH_SERIALIZATION_JIT->WITH_SERIALIZATION_JIT_ROUNDTRIP_TESTING

de155f1

Merge branch 'main' into pr/7763

01fc657

Merge branch 'main' into pr/7763

bad7fb0

This comment was marked as outdated.

Sign in to view

steven-johnson self-requested a review August 24, 2023 00:28

steven-johnson requested changes Aug 24, 2023

View reviewed changes

TH3CHARLie added 2 commits September 28, 2023 14:32

Merge remote-tracking branch 'origin/main' into xuanda/serialization-…

de8818d

…testing

use serialize to memory during jit testing

d488510

TH3CHARLie added 2 commits September 29, 2023 15:28

makefile update

ca33de5

makefile fix

1d273d3

TH3CHARLie added 2 commits October 2, 2023 14:10

skip the tutorial if flatc is not there

0428ad1

fix

441bce4

abadams reviewed Oct 5, 2023

View reviewed changes

src/Serialization.cpp Outdated Show resolved Hide resolved

TH3CHARLie added 2 commits October 5, 2023 10:40

fix signature

a538bc0

fix makefile

ef7e91b

derek-gerstmann self-requested a review October 9, 2023 20:01

derek-gerstmann approved these changes Oct 9, 2023

View reviewed changes

trigger buildbot

12583c0

TH3CHARLie merged commit e5bf7ab into halide:main Nov 6, 2023
3 checks passed

BrewTestBot mentioned this pull request Feb 2, 2024

halide 17.0.0 Homebrew/homebrew-core#161602

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add special build for testing serialization via a serialization roundtrip in JIT compilation and fix serialization leaks #7763

Add special build for testing serialization via a serialization roundtrip in JIT compilation and fix serialization leaks #7763

TH3CHARLie commented Aug 15, 2023 •

edited

Loading

steven-johnson Aug 16, 2023

TH3CHARLie Aug 16, 2023

steven-johnson left a comment

TH3CHARLie commented Aug 16, 2023

steven-johnson Aug 22, 2023

steven-johnson Aug 23, 2023

TH3CHARLie Aug 23, 2023

This comment was marked as outdated.

steven-johnson commented Aug 24, 2023

This comment was marked as outdated.

steven-johnson left a comment

TH3CHARLie commented Aug 24, 2023

derek-gerstmann commented Aug 24, 2023

abadams commented Aug 24, 2023

TH3CHARLie commented Aug 25, 2023

TH3CHARLie commented Aug 25, 2023

TH3CHARLie commented Aug 25, 2023

steven-johnson commented Sep 26, 2023

steven-johnson commented Sep 26, 2023

TH3CHARLie commented Sep 27, 2023 •

edited

Loading

TH3CHARLie commented Sep 28, 2023

TH3CHARLie commented Sep 29, 2023

abadams commented Sep 29, 2023

TH3CHARLie commented Sep 29, 2023

derek-gerstmann commented Sep 29, 2023 •

edited

Loading

derek-gerstmann commented Sep 29, 2023 •

edited

Loading

TH3CHARLie commented Oct 2, 2023

derek-gerstmann commented Oct 2, 2023

derek-gerstmann commented Oct 2, 2023

TH3CHARLie commented Oct 3, 2023

abadams Oct 5, 2023

TH3CHARLie Oct 5, 2023

derek-gerstmann Oct 5, 2023

TH3CHARLie commented Nov 6, 2023

Add special build for testing serialization via a serialization roundtrip in JIT compilation and fix serialization leaks #7763

Add special build for testing serialization via a serialization roundtrip in JIT compilation and fix serialization leaks #7763

Conversation

TH3CHARLie commented Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steven-johnson left a comment

Choose a reason for hiding this comment

TH3CHARLie commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

steven-johnson commented Aug 24, 2023

This comment was marked as outdated.

steven-johnson left a comment

Choose a reason for hiding this comment

TH3CHARLie commented Aug 24, 2023

derek-gerstmann commented Aug 24, 2023

abadams commented Aug 24, 2023

TH3CHARLie commented Aug 25, 2023

TH3CHARLie commented Aug 25, 2023

TH3CHARLie commented Aug 25, 2023

steven-johnson commented Sep 26, 2023

steven-johnson commented Sep 26, 2023

TH3CHARLie commented Sep 27, 2023 • edited Loading

TH3CHARLie commented Sep 28, 2023

TH3CHARLie commented Sep 29, 2023

abadams commented Sep 29, 2023

TH3CHARLie commented Sep 29, 2023

derek-gerstmann commented Sep 29, 2023 • edited Loading

derek-gerstmann commented Sep 29, 2023 • edited Loading

TH3CHARLie commented Oct 2, 2023

derek-gerstmann commented Oct 2, 2023

derek-gerstmann commented Oct 2, 2023

TH3CHARLie commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TH3CHARLie commented Nov 6, 2023

TH3CHARLie commented Aug 15, 2023 •

edited

Loading

TH3CHARLie commented Sep 27, 2023 •

edited

Loading

derek-gerstmann commented Sep 29, 2023 •

edited

Loading

derek-gerstmann commented Sep 29, 2023 •

edited

Loading