-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[v3.8.5] Support inline enum and mangle private variables to reduce engine size. #17703
Conversation
…s to reduce js size. Add WebGLConstants.ts for inlining enums. Depends on: cocos/cocos-ccbuild#65
…85-reduce-gfx-code
@cocos-robot run test cases |
@dumganhar, Please check the result of
Task Details
|
@dumganhar, Please check the result of
Task Details
|
@@ -105,19 +125,19 @@ class ModelBakeSettings extends EventTarget { | |||
* @en The event which will be triggered when the useLightProbe is changed. | |||
* @zh useLightProbe属性修改时触发的事件 | |||
*/ | |||
public static readonly USE_LIGHT_PROBE_CHANGED = 'use_light_probe_changed'; | |||
public static readonly USE_LIGHT_PROBE_CHANGED = ModelBakeSettingsEvent.USE_LIGHT_PROBE_CHANGED; |
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.
Will it break compatibility?
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.
No. ModelBakeSettingsEvent.USE_LIGHT_PROBE_CHANGED is assigned to 'use_light_probe_changed'
enum ModelBakeSettingsEvent {
/**
* @en The event which will be triggered when the useLightProbe is changed.
* @zh useLightProbe属性修改时触发的事件
*/
USE_LIGHT_PROBE_CHANGED = 'use_light_probe_changed',
/**
* @en The event which will be triggered when the reflectionProbe is changed.
* @zh reflectionProbe 属性修改时触发的事件
*/
REFLECTION_PROBE_CHANGED = 'reflection_probe_changed',
/**
* @en The event which will be triggered when the bakeToReflectionProbe is changed.
* @zh bakeToReflectionProbe 属性修改时触发的事件
*/
BAKE_TO_REFLECTION_PROBE_CHANGED = 'bake_to_reflection_probe_changed',
}
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.
If the value is not changed, then any benefit to use it?
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.
inline enum plugin
only deals with enums
, not the static const variables
, so static const variables
will be very long here, they take many bytes.
So add enum definition to make all places use the enums
will make the enums inlined.
Keeping the static variables here is only for compatibilty. We should not use them (static const variables) in our engine code for the best practice of smaller code size.
const CUSTOM_PIXEL_FORMAT = 1024; | ||
enum CustomPixelFormat { | ||
VALUE = 1024, | ||
} |
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.
What's the benefit to change to enum?
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.
For inlining value defined in enums.
For example:
// old code
const CUSTOM_PIXEL_FORMAT = 1024;
enum PixelFormat {
RGB_A_PVRTC_2BPPV1 = CUSTOM_PIXEL_FORMAT,
}
RGB_A_PVRTC_2BPPV1 will not be able to be inlined, and inline enum
plugin will also complain that since it could not find where CUSTOM_PIXEL_FORMAT is defined.
Currently, inline enum
rollup plugin only search all enum values, and will not search const variables
so it doesn't know what CUSTOM_PIXEL_FORMAT
is.
And if we define the private const value in another enum CustomPixelFormat
, inline enum
plugin will know that.
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.
Good to know. Does the plugin plan to support const variables?
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.
There is no plan for that, it will make the plugin more complex. There are too few usages like this in our engine code. So it's no in high priority. Keep the plugin code simpler is more important.
BTW, it plugin code is at https://github.com/cocos/cocos-ccbuild/tree/master/modules/build-engine/src/engine-js/rollup-plugins/inline-enum
@@ -749,14 +750,15 @@ export class Mat3 extends ValueType { | |||
m06 = 0, m07 = 0, m08 = 1, | |||
) { | |||
super(); | |||
const self = this; |
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.
Any benefit to use self?
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.
Answered below.
@@ -57,7 +57,7 @@ ccenum(DefaultAnimsEnum); | |||
enum DefaultCacheMode { | |||
REALTIME = 0, | |||
} | |||
ccenum(DefaultAnimsEnum); | |||
ccenum(DefaultCacheMode); |
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 modify it?
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's a bug. ccenum here is decorating DefaultCacheMode
rather than DefaultAnimsEnum
.
Normally, in our engine code, ccenum
invocation is after enum definition.
…17713) Co-authored-by: cocos-robot <[email protected]>
It's a bug starts from v3.8.5 branch: cocos#17703
It's a bug starts from v3.8.5 branch: #17703
Re: #15287
#17715
After merging this PR,package size could reduce nearly 180KB.
Inline enum
// Original code
// Inlined
Mangle variables using terser configuration
The variables with name ends with
$
will be mangled to shorter name.For example,
_myVariable$
may be mangled toae
.Changelog
Continuous Integration
This pull request:
Compatibility Check
This pull request: