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

OnceReduction: Consider calls to "once" functions in "once" functions #6055

Closed
wants to merge 20 commits into from

Conversation

kripken
Copy link
Member

@kripken kripken commented Oct 26, 2023

Say that we have a "once" function like this:

function once.A() {
  if (once.A.done) return;
  once.A.done = true;
  A();
  once.B();
}

This calls A the first time it is reached, and otherwise early-exits. It also calls
once.B, however, which may also be a once function. If it is, then we could
optimize this:

once.A();
once.B(); // unneeded

After the first call we know the second will have executed, so it can be removed.

To make this work, two small fixes were needed: First, we need to not assume
that only the "once" global itself is the only global set in a "once" function.
Second, we need to look at the proper part of the "once" function when we
consider what it is known to always set (see details in the large new comment).

@kripken kripken requested a review from tlively October 26, 2023 16:27
@kripken
Copy link
Member Author

kripken commented Oct 26, 2023

(as background, the situation of such functions calling others, and even in cycles, happens in real-world Java code)

@gkdn
Copy link
Contributor

gkdn commented Oct 26, 2023

Just want to give a heads up for following situation:

A$once() {
  if(A$done) return;
  A$done = true;
  B$once();
  C$once();
  log("executed A");  
}

B$once() {
  if(B$done) return;
  B$done = true;
  A$once();
  C$once(); // even though A$once is called before changes semantics!
  log("executed B");
}

C$once() {
  if(C$done) return;
  C$done = true;
  log("executed C");
}

If we start from A$done() this will print:

executed C
executed B
executed A

However if you remove the C$once(); call from B$once then it will print:

executed B
executed C
executed A

For this reason we don't handle this cycle situation in JsCompiler. We only handle following:

A$once() {
  if(A$done) return;
  A$done = true;
  // Condition checks above could be removed if the remaining is a single "once" call.
  B$once();
}

B$once() {
  if(B$done) return;
  B$done = true;
  ....
}

@kripken
Copy link
Member Author

kripken commented Oct 26, 2023

Thanks @gkdn , good point... We first set the global to 1 and then call code, which means recursive calls to us return early even though the full code after us has not executed. So in that example A$once is called in a recursive situation where it immediately exits, without calling C$once, so we can't remove the later C$once call.

This will require an analysis of the call graph, then.

@kripken
Copy link
Member Author

kripken commented Oct 26, 2023

@gkdn

Given:

B$once() {
  if(B$done) return;
  B$done = true;
  A$once(); // can we apply information from this call?
  C$once(); // can we remove this?
  log("executed B");
}

Then it seems like what we want, to answer the first question, is "can A$once already be on the stack?" If it can, then this call will early exit and we can not infer that things called in that function have been executed (since we might be in the middle).

But it seems like this is a problem for all functions, not just the case where B$once is a clinit function. That is, anywhere we want to infer something from a call to a clinit, we need to make sure that clinit is not already on the stack. And so we need to prove that the clinit cannot reach that position.

But given indirect calls, it seems likely we'd fail to infer in most cases, I worry. That is, if A$once does a vtable or itable call then we have to assume that a great many functions can be called, possibly covering most of the program.

Am I missing something?

@kripken
Copy link
Member Author

kripken commented Oct 26, 2023

After discussing offline with @gkdn I simplified this to handle the straightforward case that is apparently common, where a "once" function's body calls another, and has nothing else. In that case we can optimize here (but in the general case it is safer to avoid the danger of cycles and so forth; if we find a need for it later, we can investigate some call graph analysis, but as I said above, I worry about indirect calls ruining things as they always do...).

PTAL @gkdn

Comment on lines 354 to 355
// which is simple to reason about. To see why, consider that it is very
// different from this situation:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explanation of why the more complicated case doesn't work didn't really help me understand why the simple case does work. I think it would be more helpful to explain what we are going to do in the simple case more directly, then explain why it doesn't generalize to the more complicated case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I rewrote it that way.

But maybe wait to read it on seeing if @gkdn finds a fundamental problem in my code 😄

src/passes/OnceReduction.cpp Outdated Show resolved Hide resolved
@gkdn
Copy link
Contributor

gkdn commented Oct 27, 2023

@kripken This doesn't optimize the scenario that I was handling on my side. It looks like it doesn't have the same effect. I will take a look what is going on.

Co-authored-by: Thomas Lively <tlively@google.com>
@gkdn
Copy link
Contributor

gkdn commented Oct 27, 2023

I disabled partial inlining and this is the left-over code:

 (func $$clinit__void@java.util.logging.Level (type $0)
  (if
   (global.get $f_$initialized__java_util_logging_Level)
   (return)
  )
  (global.set $f_$initialized__java_util_logging_Level
   (i32.const 1)
  )
  (call $$clinit__void@java.util.logging.Level.LevelAll)
  (call $$clinit__void@java.util.logging.Level.LevelConfig)
  (call $$clinit__void@java.util.logging.Level.LevelFine)
  (call $$clinit__void@java.util.logging.Level.LevelFiner)
  (call $$clinit__void@java.util.logging.Level.LevelFinest)
  (call $$clinit__void@java.util.logging.Level.LevelInfo)
  (call $$clinit__void@java.util.logging.Level.LevelOff)
  (call $$clinit__void@java.util.logging.Level.LevelSevere)
  (call $$clinit__void@java.util.logging.Level.LevelWarning)
 )
 
 
 (func $$clinit__void@java.util.logging.Level.LevelAll (type $0)
  (if
   (global.get $f_$initialized__java_util_logging_Level_LevelAll)
   (return)
  )
  (global.set $f_$initialized__java_util_logging_Level_LevelAll
   (i32.const 1)
  )
  (call $$clinit__void@java.util.logging.Level)
 )
 
 (func $$clinit__void@java.util.logging.Level.LevelConfig (type $0)
  (if
   (global.get $f_$initialized__java_util_logging_Level_LevelConfig)
   (return)
  )
  (global.set $f_$initialized__java_util_logging_Level_LevelConfig
   (i32.const 1)
  )
  (call $$clinit__void@java.util.logging.Level)
 )
 
 // others the same

In my own handling I rewrite for example

 (func $$clinit__void@java.util.logging.Level.LevelAll (type $0)
  (if
   (global.get $f_$initialized__java_util_logging_Level_LevelAll)
   (return)
  )
  (global.set $f_$initialized__java_util_logging_Level_LevelAll
   (i32.const 1)
  )
  (call $$clinit__void@java.util.logging.Level)
 )

as

 (func $$clinit__void@java.util.logging.Level.LevelAll (type $0)
  (call $$clinit__void@java.util.logging.Level)
 )

which later gets inlined to parent clinit call.

@kripken
Copy link
Member Author

kripken commented Oct 27, 2023

@gkdn Makes sense, that also looks like a valid optimization. I'd like to do it in a followup PR to this however, to keep this one as simple as possible (the comments are already dozens of lines 😄 ).

@gkdn
Copy link
Contributor

gkdn commented Oct 27, 2023

@kripken I think the optimization make sense but I'm not sure if it will have any extra value if we do the other optimization. IIUC this extra information will not be useful with the rewriting as it will provide the same information. I tested both optimizations together and it looks like the code size is exactly the same but I might be missing something.

@kripken
Copy link
Member Author

kripken commented Oct 27, 2023

Yeah, I think you're right, after other optimizations they end up equal. I implemented the other one and compared them on the tests from this PR:

https://gist.github.com/kripken/56a0f4f1d70e9575273c154d6eb981f8

The difference is only in cases that inlining would end up helping out later. So this PR only helps accelerate the process. As it is fairly complex in the necessary comments, maybe it isn't worth it.

I'll open a PR with the other one next week after I polish and test the code.

@kripken
Copy link
Member Author

kripken commented Oct 30, 2023

Replaced by #6061

@kripken kripken closed this Oct 30, 2023
@kripken kripken deleted the once.moar branch October 30, 2023 22:22
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.

3 participants