halide / Halide

a language for fast, portable data-parallel computation
https://halide-lang.org
Other
5.91k stars 1.07k forks source link

Deprecate vs Remove `stmt_html` output for Halide 16? #7507

Closed steven-johnson closed 1 year ago

steven-johnson commented 1 year ago

With #7421 having landed, there is really not much use for the stmt_html output any more.

Ordinarily I'd say we should deprecate it, but (1) it will be hard to emit useful deprecation warnings for all use cases, and (2) I don't recall seeing a single use of it inside Google for several years now -- I wouldn't be surprised if the same is true externally.

So, taking a straw poll here -- does it make more sense to just rip off the band-aid, so to speak, and remove it entirely for Halide 16?

shoaibkamil commented 1 year ago

I think it's ok to remove it, but we may want to alias stmt_html to output stmt_viz or at least provide a reasonable error message-- but that seems just as hard as a deprecation message.

slomp commented 1 year ago

One thing to consider: compared to stmt_html, how long does it take to generate? And also, how long does it take to open it in a browser?

abadams commented 1 year ago

I propose that to a user of the releases, it should just appear as if the html output got better. So we don't introduce any "viz" names at all (which already collide with HalideTraceViz), and just swap in the new html output and delete the old one at the same time.

steven-johnson commented 1 year ago

Tagging @maaz139 as his input would be welcomed here

maaz139 commented 1 year ago

One thing to consider: compared to stmt_html, how long does it take to generate? And also, how long does it take to open it in a browser?

The new HTML is definitely slower to load. In general, html files above ~15mb were not working well in my experiments. I think there are ways to improve the HTML performance and it would be worth doing so if people are actively using the feature and running into this issue.

Other than performance, I don't see a reason to keep the old html file around. I would advocate silently swapping the new viz.html for the old html tags as Andrew suggested.

antonysigma commented 1 year ago

Chiming in... As an Nvidia GPU programmer, I am in favor of the new StmtViz output as long as we can bring back the gpu_threads, gpu_blocks IR visualization back in Halide 10. Ever since I moved to Halide 14, all the scheduled GPU code are visualized as assembly code.

The strength of StmtViz tool is to compare the IR and assembly code side by side, with cross referencing. However, if the IR visualization panel dumps the assembly code like what we do in Halide 14, it kind of defeats the purpose, right?

The figure below shows how a generic code blurred_x , scheduled with gpu_theads, is visualized in Halide 10 and Halide 14.

image

maaz139 commented 1 year ago

Thanks for the feedback Antony! I agree, StmtViz should show IR code now that we have a separate assembly tab. I'll open an issue and look into it.

abadams commented 1 year ago

For that to happen we'd have to emit the .stmt file before the offload gpu pass, e.g. in Lower.cpp around line 441. I guess we'd have to stash that earlier stmt in the Module somewhere for use in the .stmt outputs.

steven-johnson commented 1 year ago

As written, this issue is fixed, closing. (I presume we'll open a new issue for @antonysigma's request)

mcourteaux commented 1 year ago

I want the original HTML statement file back. The new version is SUPER slow in the webbrowser.

maaz139 commented 1 year ago

Perhaps we can add lazy rendering for the visualization / assembly tabs. This should, in theory, restore the page responsiveness and load times to previous levels until the user explicitly requests the rendering of the new tabs.

mcourteaux commented 1 year ago

image

This message stays in the lower-left corner for me for a long while. When I check the network tab, it says that all the resources are cached, so I don't know what it's doing. Parsing the javascript file?

steven-johnson commented 1 year ago

Out of curiosity, what browser / hardware etc are you using? (I'm not aware that anyone else involved in the review of this noticed a meaningful slowdown, so it may be something specific to your setup that we can address somehow)

mcourteaux commented 1 year ago

Out of curiosity, what browser / hardware etc are you using? (I'm not aware that anyone else involved in the review of this noticed a meaningful slowdown, so it may be something specific to your setup that we can address somehow)

An 48GB memory, 8-core machine.

image

Running Mozilla Firefox 115.0.2

Lemme try this in Chromium real quick... Same for Chromium: it took 12 seconds for the page to load.

Feel free to try this for yourself: neonraw_denoiser_f8_k7_p1-x86-64-linux-avx2-fma-profile_by_timer.stmt.html.zip

antonysigma commented 1 year ago

Same for Chromium: it took 12 seconds for the page to load.

@mcourteaux I am afraid the Halide team (as well as me) are not too familiar with the Javascript and ES6. Have you considered reaching out to the original author of the PR?

Core HTML5 developers tend to bundle all scripts with toolchains like the esbuild or webpack, so as to embed the entire web application as one offline script file bundled.js.

Chiming in... Could you please post a screenshot of the waterfall diagram representing the load times? This is what I have on my machine, rendering the same HTML you uploaded earlier:

image

mcourteaux commented 1 year ago

In Chromium, I can record a profile:

image

(The waterfall for network resources is useless, it's instantly loaded. The bottleneck is indeed some script running).

The script in question is the one that starts inline at line 8898. The call stack is this very non-helpful thing... image

mcourteaux commented 1 year ago

Aha! I caught a very useful profile! image All the time is spent in "init_tooltips" which is just waiting 100% of the time on jQuery selectors.

I'll upload it here. Trace-20230726T110312.json.zip Extract, and open it with Chromium in the Performance tab of the Developer Tools.

mcourteaux commented 1 year ago

I'll move this to a new issue and tag the right people.

mcourteaux commented 1 year ago

Opened #7712