-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
cgen: move closure code to vlib #24912
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
Conversation
Connected to Huly®: V_0.6-23381 |
benchmark result: before this PR: V Language Closure Performance Benchmark Report1. Closure Performance Analysis
2. Memory Overhead Analysis
Verification Sum: -900 (Calculated from random sample of 100 closures) Test Environment
After this PR: V Language Closure Performance Benchmark Report1. Closure Performance Analysis
2. Memory Overhead Analysis
Verification Sum: -900 (Calculated from random sample of 100 closures) Test Environment
|
vlib/builtin/closure.v
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am against this being directly in builtin
, since it will affect even programs that do not use closures at all.
It could be in a builtin.closure
module instead, that gets imported selectively, similar to how sync
is imported for channels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will also solve the problem of more exotic environments breaking (like freestanding etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By moving it in builtin.closure
, I encounter some errors:
FAIL [ 35/2449] C: 1512.3 ms, R: 0.000 ms vlib/builtin/js/array_test.js.v
>> compilation failed:
builder error: error: import cycle detected between the following modules:
* builtin -> builtin.closure -> builtin
* main -> builtin -> builtin.closure -> builtin
* builtin.closure -> builtin -> builtin.closure
* os -> builtin -> builtin.closure -> builtin
* strings.textscanner -> builtin -> builtin.closure -> builtin
FAIL [ 36/2449] C: 1420.1 ms, R: 0.000 ms vlib/builtin/js/string_test.js.v
>> compilation failed:
builder error: error: import cycle detected between the following modules:
* builtin -> builtin.closure -> builtin
* main -> builtin -> builtin.closure -> builtin
* builtin.closure -> builtin -> builtin.closure
* os -> builtin -> builtin.closure -> builtin
* strings.textscanner -> builtin -> builtin.closure -> builtin
OK [ 37/2449] C: 214.7 ms, R: 1.441 ms vlib/builtin/wchar/wchar_test.v
FAIL [ 38/2449] C: 1542.9 ms, R: 0.000 ms vlib/builtin/js/int_test.js.v
>> compilation failed:
builder error: error: import cycle detected between the following modules:
* builtin -> builtin.closure -> builtin
* main -> builtin -> builtin.closure -> builtin
* builtin.closure -> builtin -> builtin.closure
* os -> builtin -> builtin.closure -> builtin
* strings.textscanner -> builtin -> builtin.closure -> builtin
OK [ 39/2449] C: 244.9 ms, R: 1.559 ms vlib/cli/command_test.v
debugging.......
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try adding builtin.closure
in builtin_module_parts
in vlib/v/util/util.v:17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although that may need adding that change separately in a single PR, so that the compiler can bootstrap later with it on master
Interesting ... why is there a diff for the |
if p.file_backend_mode == .v || p.file_backend_mode == .c { | ||
p.register_auto_import('builtin.closure') | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not all anonymous functions are closures ... many of them do not capture anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably make this more specific in the future 🤔
if p.file_backend_mode == .v || p.file_backend_mode == .c { | ||
p.register_auto_import('builtin.closure') | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this either ... the parameter may be a simple anonymous function, that does not capture anything and does not need allocating thunks etc.
@@ -284,6 +284,22 @@ fn (mut p Parser) struct_decl(is_anon bool) ast.StructDecl { | |||
// error is set in parse_type | |||
return ast.StructDecl{} | |||
} | |||
|
|||
// for field_name []fn, cgen will generate closure, so detect here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would cgen generate a closure for a field init?
afaik it only does it for stuff like field: instance.method
, not for normal functions, whether anonymous or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work.
* scanner: fix multi-level string interpolation in if/match branch * scanner: fix multi-level string interpolation in if/match branch, fix test code * Update vlib/v/scanner/scanner.v Co-authored-by: Delyan Angelov <[email protected]> * rename the rest * remove vfmt off/on tags * cgen: fix array data for option array/fixed array(?[]u8/?[3]u8) * cgen: fix fixed array with type alias * cgen: fix fixed array with type alias * cgen: move sort fn after interface definitions * parser: fix syntax error for c in [othermod.Struct{field: 255}] { * comptime: support string interpolation in `compile warn/error` * comptime: support string interpolation in `compile warn/error` * fix error for string literal * make level less than 0 for compile warn/error * cgen: move closure C code to V code under vlib/builtin/closure/ (vlang#24912) * ci: workaround -usecache issue afte 2d87ac4 * markused,checker: fix hello world size after the introduction of `builtin.closure` in 2d87ac4 (vlang#24989) * time: fix more panics in the supported specifiers in Time.custom_format/1 (vlang#24988) --------- Co-authored-by: krchi <[email protected]> Co-authored-by: Delyan Angelov <[email protected]> Co-authored-by: kbkpbot <[email protected]>
* scanner: fix multi-level string interpolation in if/match branch * scanner: fix multi-level string interpolation in if/match branch, fix test code * Update vlib/v/scanner/scanner.v Co-authored-by: Delyan Angelov <[email protected]> * rename the rest * remove vfmt off/on tags * cgen: fix array data for option array/fixed array(?[]u8/?[3]u8) * cgen: fix fixed array with type alias * cgen: fix fixed array with type alias * cgen: move sort fn after interface definitions * parser: fix syntax error for c in [othermod.Struct{field: 255}] { * comptime: support string interpolation in `compile warn/error` * comptime: support string interpolation in `compile warn/error` * fix error for string literal * make level less than 0 for compile warn/error * cgen: move closure C code to V code under vlib/builtin/closure/ (vlang#24912) * ci: workaround -usecache issue afte 2d87ac4 * markused,checker: fix hello world size after the introduction of `builtin.closure` in 2d87ac4 (vlang#24989) * time: fix more panics in the supported specifiers in Time.custom_format/1 (vlang#24988) --------- Co-authored-by: krchi <[email protected]> Co-authored-by: Delyan Angelov <[email protected]> Co-authored-by: kbkpbot <[email protected]>
This PR:
cgen
tovlib/builtin
.vlib/bench/bench_closure.v
TODO:
freestanding can't work now because lack of
pthread_mutex