-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Add --inlineConsts
flag
#6805
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
Comments
Is this only expected to inline strings/numbers? Regex literals? Template strings? |
Only "compile-time computable constants", i.e. all the things that are valid |
In the above example, the declaration of the const disappears. That could cause problems in a Given a namespace: // vec3.ts
namespace vec3 {
export const SIZE = 3
export function create(x, y, z) {...}
} And code that uses it, expecting the constant to be there: // buffer-helpers.ts
interface VectorType {
SIZE: number
create: Function
}
function read(buffer: Float32Array, type: VectorType) {
// do something that uses type.SIZE
}
read(someBuffer, vec3) Output code, following the suggested rules: // app.js
(function(vec3) {
vec3.create = function(x, y, z) {...}
})(vec3)
function read(buffer, type) {
// do something that uses type.SIZE
}
read(someBuffer, vec3) // will fail in some unexpected manner due to missing vec3.SIZE |
Hrm, good point. Maybe it's only safe to remove |
Is the goal to optimize by file size, load/parse speed, or execution speed? |
constant folding usually targets execution speed. the file size will probably increase if you have that constant used in a few places. |
Ideally, it could still inline uses of a module-scope constant, even if it keeps the declaration there. Would that be sensible?
What would be a suitable way to determine which functions are compile-time computable? Would it be enough to say any method on String/Array/Number/Boolean that doesn't have "locale" in the name? |
@RyanCavanaugh On #6805 (comment), and #6805 (comment) maybe we should add some new comment form like: /* (const) {ConstantName} ({ConstantValue}) */. So: // vec3.ts
namespace vec3 {
export const SIZE = 3
export function create(x, y, z) {...}
} Would be adjusted to be: // vec3.ts
/* (const) vec3.SIZE (3) */
namespace vec3 {
export const SIZE = 3
export function create(x, y, z) {...}
} For completeness I included the sample client code // buffer-helpers.ts
interface VectorType {
SIZE: number
create: Function
}
function read(buffer: Float32Array, type: VectorType) {
// do something that uses type.SIZE
}
read(someBuffer, vec3) The compiler would see the comment and emit some helper like this: var __With = function(Target, Name, Value)
{
Target[Name] = Value;
return (Target);
}; Which would be used like: (function(vec3) {
vec3.create = function(x, y, z) {...}
})(vec3)
function read(buffer, type) {
// Other code
while (SomeVar < type.SIZE)
{
/* Code */
}
// Other code
}
read(someBuffer, __With (vec3, "SIZE", 3)) Could that work or am I just missing all the corner cases? |
@KingDavid12 Your generated code is semantically equivalent to the below: vec3.create = function(x, y, z) {...}
function read(buffer, type) {
// Other code
while (SomeVar < type.SIZE)
{
/* Code */
}
// Other code
}
read(someBuffer, (vec3.SIZE = 3, vec3)) There's 0 constant folding going on here, so you don't have the runtime benefit of minimal computation. |
Should strings be excluded from the constant propagation after a certain limit? There's usually minimal perf difference in practice, and larger strings sometimes are faster when stored in a variable on the heap. Not arguing numbers, etc., though. Those can still be folded as normally. |
This feature can actually decrease file size sometimes when combined with tools like uglify. Usually with conditionals: const myConst = "blah";
// ...
// this code can be safely removed after myConst gets replaced with "blah"
if (myConst !== "blah") {
// ...
} |
@isiahmeadows Good point. Maybe what can be done is to inline constants internally in the namespace but leave the definition. That would allow later code using the library to see the constant definition and inline it themselves. |
On Sat, Oct 29, 2016, 11:02 KingDavid12 notifications@github.com wrote: @isiahmeadows https://github.com/isiahmeadows Good point. Maybe what can That would have to be done to some extent regardless, in order to properly — Reply to this email directly, view it on GitHub |
A const string enum would seem to do this pretty well? const enum X { x = "hello" }
console.log(X.x); becomes: "use strict";
console.log("hello" /* x */); |
i 'd like to work on this i'm going to add a inlineConstantTransformer or add transform on tsTransformer resolve and mark identifier if it can be inline update identifier node to resolved node Is there any problem with this? need help plz 🙏 |
I think we want to re-evaluate the necessity of this |
What about not solving this with a separate flag? I'd suggest putting the // this variable exists somewhere, isn't inlined
declare const hello;
// we want to inline this function's implementation
declare const isHello = (v): v is 'hello' => v === 'hello';
// this is inlined later
declare const world = 'world';
if (isHello(hello)) {
console.assert(hello[0] === 'h')
console.log(hello, world)
} if (hello === 'hello') {
console.assert(hello[0] === 'h')
console.log(hello, 'world')
} There's clearly a use-case for this. For example, I write many utility functions just for type-safety, most of which would evaluate to I would not want to inline ALL // someObject.ts
const someObject = {
type: 'object'
};
export default someObject; // index.ts
import someObject from 'someObject';
let obj = someObject; // default value for `obj`
if (/* some condition */) {
// assign something to `obj`
}
// if `obj` is still its default value
if (obj === someObject) {
obj = something;
}
// etc. After inlining consts, using proposed EDIT: here's a playground with a small executable demo of what I mean. Of course, it's possible to maybe not inline consts that are used like this... but won't it be too much cognitive load to always remember which of your consts are inlined and which are not? The alternative use of This would actually behave a lot like C++ |
@Raiondesu In my experience, I've never once stumbled across a use case where a primitive constant would've been better not inlined, even in perf-critical code. If you're worried about duplication, gzip can make quick work of that. About the only two places I could imagine it being useful to not inline a constant is in embedded applications and with very large strings used in multiple unrelated places. In the first, you need enough control that you'd do better (ab)using |
@isiahmeadows, I understand your point. It's fair. But...
In my comment I never stated that I'm concerned about code duplication or performance (however your case of randomly placed long strings is actually pretty common and would benefit from a keyword solution). My case was that it would be useful, if consts could inline not only simple primitives, but also complex expressions (like functions or objects). And that it also could be useful if you could atomically control which consts need to be inlined. If in your practice it's actually useful to inline all of them - I don't see any problem with In the case of a compilation flag, however, it would not only break the vanilla js behavior (I mentioned it in the previous comment), but also make it a complete disaster for readability, since you'd never know by heart (intuitively) which consts in your code are inlined and which aren't. One of TS goals is to not break the semantics of vanilla js. And introducing the If adding some semantics to |
I have another use case for this suggestion, when browser-facing code need reference constants include const list and object which inside module that not designed for browser. // index.js of module foo which is huge
export const bar = [1,2,3] as const // module that intended to run in browser
import { bar } from 'foo'
if (bar.include(x)) ... which on compile with --inlineConsts would emit code
this can avoid import huge module to browser, while no need to make hard copy of bar to code |
This is clearly out of scope. Bad idea, Ryan. |
New flag
--inlineConsts
Input:
Output:
See also #6678 + #6804
TBD: It's not clear you would want longer strings to inline into their use sites. Discuss.
The text was updated successfully, but these errors were encountered: