Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Expose a profiler API #229

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

Matyrobbrt
Copy link

@Matyrobbrt Matyrobbrt commented Jul 17, 2022

This PR exposes a profiler API in spark-api.

Changes made

  • Added a new subproject, spark-proto which all other projects depend on (including the API). This allows the API to access the java object of reports
  • Added a new subproject, api-test, which is a test Forge mod for testing the API.
  • Different classes in the me.lucko.spark.common.sampler package have been moved in the API, some of them as interfaces with hidden implementations using Spark
  • SamplerModule has been changed to use the implementation of the API

Maven setup

As configured in spark-api/build.gradle and spark-proto/build.gradle, I recommend the following maven structure:

  • base group me.lucko.spark
  • Proto in me.lucko.spark:spark-proto shading the proto library
  • API in me.lucko.spark:spark-api, depending on me.lucko.spark:spark-proto

API Usage

Example API usage:

private static void profile() throws Exception {
        final Spark api = SparkProvider.get();
        final Profiler profiler = api.profiler();

        // Infinite report
        {
            final Profiler.Sampler sampler = profiler.createSampler(ProfilerConfiguration.builder()
                    .dumper(ThreadDumper.ALL)
                    .grouper(ThreadGrouper.AS_ONE)
                    .ignoreNative()
                    .build(), (e, msg) -> System.err.println("Error creating sampler: " + e + ": " + msg));
            if (sampler == null)
                throw new AssertionError();

            sampler.start(); // Start the profiler
            Thread.sleep(1000 * 60 * 5); // Wait 5 minutes
            sampler.stop(); // Stop the profiler

            final ProfilerReport report = sampler.dumpReport(ReportConfiguration.builder()
                    .comment("My comment")
                    .sender("Me", UUID.randomUUID())
                    .build());
            System.out.println("Report 1 uploaded to " + report.upload());
            report.saveToFile(Path.of("report1.sparkprofile")); // Save the report
        }

        // Report with set time
        {
            final Profiler.Sampler sampler = profiler.createSampler(ProfilerConfiguration.builder()
                    .dumper(new SpecificThreadDumper(Thread.currentThread()))
                    .grouper(ThreadGrouper.BY_NAME)
                    .ignoreSleeping()
                    .samplingInterval(12)
                    .forceJavaSampler()
                    .duration(Duration.ofMinutes(10))
                    .build(), (e, msg) -> System.err.println("Error creating sampler: " + e + ": " + msg));
            if (sampler == null)
                throw new AssertionError();

            sampler.start(); // Start the profiler
            sampler.onCompleted(ReportConfiguration.builder()
                    .separateParentCalls(true).build())
                    .whenComplete((report, throwable) -> {
                        if (throwable != null)
                            throw new RuntimeException(throwable);
                        System.out.println("Report 2 uploaded to " + report.upload());
                    }); // Configure the dump after 10 minutes, when the sampler is done
        }
}

Effectively supersedes #131.

@embeddedt
Copy link

+1. I would like to use Spark to profile the integrated server startup (i.e. before I can enter a command) and this would be very useful.

@Matyrobbrt
Copy link
Author

The PR is ready for review.

@lucko
Copy link
Owner

lucko commented Aug 29, 2022

Hi, thanks for the PR! I will try to have a deeper look at this soon.

I will be totally honest/upfront with you, I have a few reservations about merging this currently:

  • Systems that automatically trigger the profiler and upload the data are already causing a problem for me in terms of data storage (remember that spark data is uploaded centrally). I am concerned that this is only going to get worse if we add an API for it. I would at least like to get that under control (it is barely at the moment) & fully understand the $ costs before we add a feature to accelerate the data usage 😛
  • I would prefer to not have implementation classes in the API package/module. It should contain only interfaces and enums ideally.
  • If the API is tightly coupled to the implementation (as it seems to be at the moment), it makes it more difficult to make changes to spark-common and the platforms in the future, which may hinder improvements or new functionality down the line.

That's where I'm at atm :) please feel free to ping/chat to me on Discord (https://discord.gg/PAGT2fu) if you have any comments or suggestions, or of course you can reply here.

@Matyrobbrt
Copy link
Author

Moved discussion to Discord

@ArDiDEx
Copy link

ArDiDEx commented May 8, 2023

hey, i'm curious if this is still planned to be added or has been cancelled

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants