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

[hlc] Add jumbo build for vs template #737

Merged
merged 3 commits into from
Jan 15, 2025

Conversation

yuxiaomao
Copy link
Collaborator

Add jumbo build (aka unity build but I prefer avoid using "unity") for hlc templates and use it by default for better hlc compilation speed with Visual Studio (especially when use hl/jit for dev and hl/c for release).
If incremental build is preferred, jumbo build can be disabled by setting -D hlgen.nojumbo.

@tobil4sk made a couple of tests and it turns out that this all-in-one build strategy is still the quickest and is a known approach. It's already used internally in Shiro's project but was made by manually editing vcsproj file.

@skial skial mentioned this pull request Jan 6, 2025
1 task
@tobil4sk
Copy link
Member

tobil4sk commented Jan 6, 2025

Should we maybe have a single define for enabling or disabling them? Something like this, but I'm not sure if there is a standard for using yes/no in haxe defines yet:

-D hlgen.jumbo=yes
-D hlgen.jumbo=no

This will make it easier to have a different default value for this define based on which template we are using, as the trade off will be different for other compilers.

Currently, these template related defines all have makefile in the name, should we continue with this standard? -D hlgen.makefile.jumbo?

It may also make sense to rename the HL_MAKE define accordingly to something like HL_JUMBO_BUILD which makes more sense with the new behaviour (using an "hlgen.makefile" template no longer implies HL_MAKE will be set). This would require changing the generated hlc and both would have to be supported for now.

@yuxiaomao
Copy link
Collaborator Author

I don't think =yes / =no is standard, but we can use something like that (or enable/disable). However I'm not sure how we're supposed to configure this default value for a given template: perhaps by sending both "forceJumbo" "forceNoJumbo" variables?

This would require changing the generated hlc and both would have to be supported for now.

I'm not sure that I understand you here. HL_MAKE is actually HL_NO_JUMBO and is only used by VS template for now. For your PR adding a makefile incremental build template, it can just define HL_NO_JUMBO too.

@tobil4sk
Copy link
Member

tobil4sk commented Jan 7, 2025

However I'm not sure how we're supposed to configure this default value for a given template: perhaps by sending both "forceJumbo" "forceNoJumbo" variables?

We can set jumboBuild based on the value of hlgen.makefile.jumbo if it exists. Then if we store the context as a variable context, we can have a $$setJumboBuildDefault() macro like this:

setJumboBuildDefault: function(c, boolString:String) {
	final value = switch boolString {
		case "true": true;
		case "false": false;
		default: throw 'setJumboBuildDefault value must be true or false';
	};

	context.jumboBuild ??= value;
	return "";
}

So if hlgen.makefile.jumbo is given it is always respected, but otherwise the template can choose a default value.

HL_MAKE is actually HL_NO_JUMBO and is only used by VS template for now.

Yes, I got them the other way around. I think the original intention behind the HL_MAKE flag was to allow two types of compilation:

  • Direct compilation via a command line (e.g. gcc sample command on hashlink website). To simplify this for a user, the user does not need an extra define and can just pass a single file (main.c) which #includes everything else.
  • Compilation via a template. Since the template is auto generated and it is aware of all the files needed, it can just specify all of them and it can have HL_MAKE set by default.

However, now we want to allow templates that also put everything into a single file for the benefits of the jumbo build, so this connection between using hlgen.makefile and HL_MAKE is no longer there.

We can change hlc gen to write:

#if !defined(HL_MAKE) || defined(HL_JUMBO)

which separates the concept of a jumbo build from a "template" build. Then all templates set HL_MAKE, but only some templates define HL_JUMBO.

<PreprocessorDefinitions>_DEBUG;_CONSOLE;HL_MAKE;::if jumboBuild::HL_JUMBO;::end::;%(PreprocessorDefinitions)</PreprocessorDefinitions>

This way, we allow HL_MAKE to stay consistent with its original purpose and it is a bit less confusing.

@yuxiaomao
Copy link
Collaborator Author

Changed to -D hlgen.makefile.jumbo=true with setDefaultJumboBuild(true) (name can be adjust). Comparing bool in template context after set doesn't work well for me, so it's currently just a string.

I agree that HL_MAKE does not describe what it does now, but it would cause compatibility problem and would require update haxe too.

@tobil4sk
Copy link
Member

tobil4sk commented Jan 8, 2025

Changed to -D hlgen.makefile.jumbo=true with setDefaultJumboBuild(true) (name can be adjust). Comparing bool in template context after set doesn't work well for me, so it's currently just a string.

This looks good!

I agree that HL_MAKE does not describe what it does now, but it would cause compatibility problem and would require update haxe too.

Perhaps in the future we can sort this out in a backwards compatible way by checking the version in hlc.json.

I also wonder if the precompiled headers are useful at all with a jumbo build? Maybe that should be checked.

@yuxiaomao
Copy link
Collaborator Author

yuxiaomao commented Jan 9, 2025

I just tested with a large project, it takes 59-62s in a jumbo build with precompiled header; and 62-64s without precompiled headers (I don't understand why, but I'll keep precompiled header for that).

@yuxiaomao
Copy link
Collaborator Author

As jumbo is a string now, would it make more sens to have -D hlgen.makefile.mode=jumbo and $$setDefaultMode(jumbo)? It can have other values such as nojumbo (or incremental). This leaves the possibility to support other "modes" in a templates - I can't see a real use case for msvc templates now and it's not easy to handle, but for example "flat"

@tobil4sk
Copy link
Member

This leaves the possibility to support other "modes" in a templates

I think this will have problems because we might want to enable jumbo at the same time as some other mode, but with this method we would only ever be able to enable one at a time. I think a boolean flag separate from anything else makes most sense here.

@yuxiaomao
Copy link
Collaborator Author

Ok. I'm not very satisfied with the comparison using == "true", but I didn't find a better solution. Let's say this PR is ok for me now.
Will merge next week if no one is against.

@yuxiaomao yuxiaomao merged commit d8b9dcd into HaxeFoundation:master Jan 15, 2025
10 checks passed
@yuxiaomao yuxiaomao deleted the dev-jumbotemplate branch January 15, 2025 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants