Server mode and templated views

[smellsblue]

This is a major refactoring adding a couple big changes: server mode, and templated views, and some speed improvements.

The server mode allows dump files to be loaded and unloaded at will, so you can run fasthat once, then use it when you need it. I added logging so the server marks when requests happen and how long they take.

Templated views shifts all the views from view code intermixed with the server code into separated Mustache templates from the server side code that supports them. This allows a single layout that can more easily be manipulated, partial templates for common view code, and keeps html in a language designed for html rather than embedded in the java. This also paves the way for future improvements like better overall navigation via a menu bar, bootstrapping the views, and other possibilities.

While I was at it, I drastically improved the performance of the histogram pages by precaching some data during the dump loading phase.

[cky]

This diff is massive! So that you can comment on my comments so far, I'll submit this current batch of comments.

[cky]

Should this add a -histo option too?

[smellsblue]

👍

[cky]

Is there any circumstance where this condition would be true? Since i starts at 0, and the loop is entered only if args.length > i, that implies that args.length > 0. (I am, of course, assuming that args is never rebound, and so args.length never changes between loop iterations.)

[smellsblue]

👍

[cky]

With your new for loop, this condition is also obsolete.

[smellsblue]

👍

[cky]

This is not secure. For example, new File("/usr/include/../bin/gcc").toPath().startsWith(new File("/usr/include").toPath()) evaluates to true.

[smellsblue]

👍

[cky]

This method should be named isParseOnly.

[smellsblue]

👍

[cky]

I know a lot of this is cut-and-paste, but when I see println calls with variable interpolation, I try to convert to the equivalent printf call (with a %n at the end of the format string to print a newline). Yes, I wish Java has real interpolated strings too.

[smellsblue]

👍 (also converted all other println calls I could find)

[cky]

This is another case where using a Guava memoising supplier is more ideal.

[smellsblue]

👍

[cky]

Please use printf(...) (with a %n at the end of the format string) instead of println(format(...)).

[smellsblue]

👍

[cky]

Please use printf(...) (with a %n at the end of the format string) instead of using println(format(...)).

[smellsblue]

👍

[cky]

Instead of working with spliterators, consider refactoring JavaClass.getInstances to return a stream. The Stream.concat method could be put to very good use here.

[smellsblue]

👍

[cky]

Here's my next batch of comments. There's still a lot of review left to do, so I'll continue on Thanksgiving. :-D

[cky]

I know this is cut-and-pasted, but you can just write Object[] args = { wrapJavaObject(obj) };.

[smellsblue]

👍

[cky]

The new Object[] { ... } is also redundant here, since call is a variadic method.

[smellsblue]

👍

[cky]

May I suggest using Guava's Stopwatch class to handle all the elapsed-time-handling stuff?

[smellsblue]

👍

[cky]

IntelliJ IDEA sez: "Method isOqlSupported() is never used"

😉

More seriously, the fact that this method is used purely for Mustache should probably be documented. May I suggest an annotation such as @ViewGetter? (Feel free to come up with a better name.)

[smellsblue]

👍

[smellsblue]

I implemented this as the @ViewGetter annotation. I do want to add that I think this annotation has a big risk of becoming outdated. I think Mustache is flexible enough that we could customize it such that the methods called would be required to have this annotation, but I don't think it would be worth it just to enforce @ViewGetter is added to the method. Besides, you could still remove a call to a particular method but leave the annotation.

Without the enforcement, I think it's easy to either add a new method to be used by the view and forget to add the annotation, or remove a call from a view and forget to drop the annotation if it is no longer actually used. The annotation will be essentially a hint that the methods are called from the view, which I suppose is what you are going for.

I'll do my best to keep it up to date with any changes I make, but I wanted to express the concern 😃

[cky]

Yes, the annotation is what I was going for. Although we don't currently have tooling to enforce this, I do envision writing an IntelliJ plugin to actually parse the Mustache files and see what's being called:

1. It will red-squiggle anything the Mustache file invokes that's not implemented Java-side
2. It will warn about unused @ViewGetter properties implemented Java-side that's not being used Mustache-side

Granted, @ViewGetter annotations are not required per se to implement that sort of plugin. So this is more of a human reading aid, I guess. ;-) I would probably add a tag in all the Mustache files to say which view model class it uses, which the plugin would use to do all the necessary linkages. But we'll cross that bridge when we get there.

[cky]

IntelliJ IDEA sez: "Anonymous new MustacheResolver() can be replaced with method reference."

😉

It performed the following refactoring:

private static final MustacheFactory MUSTACHE_FACTORY = new DefaultMustacheFactory(MustacheQueryHandler::getReader);

[smellsblue]

👍

[cky]

Here's my next batch of review comments. Enjoy! Sorry if some items are too nitpicky for your liking. ;-)

[cky]

Personally, I would prefer to create a ModelValueVisitor<T> (or the like) that is just like ModelVisitor but will return a type T. Then we add a visit() overload to accept a ModelValueVisitor<T>.

If we decide not to go down that path, we should at the very least create a Ref<T> class like we have in our other codebases, to avoid using the one-element array hack.

[smellsblue]

👍

[cky]

I would highly suggest using a Guava memoised supplier instead of doing manual memoisation. For starters, it makes this method instantly thread-safe. Here's my implementation:

public Model getModel() {
    if (handler.isRawMode()) {
        return null;
    }

    return modelSupplier.get();
}

private Model getModelInternal() {
    return handler.getSnapshot().getModelFactories().stream()
            .map(factory -> factory.newModel(thing))
            .filter(Objects::nonNull)
            .map(this::simplifyModel)
            .findFirst()
            .orElse(null);
}
    
private Model simplifyModel(Model model) {
    if (!useSimpleForm) {
        return model;
    }
    if (model instanceof HasSimpleForm) {
        return ((HasSimpleForm) model).getSimpleFormModel();
    }
    return null;
}

where modelSupplier is defined as follows:

private final Supplier<Model> modelSupplier = Suppliers.memoize(this::getModelInternal);

[smellsblue]

👍

[cky]

This null check is redundant, because null is never instanceof anything.

[smellsblue]

👍

[cky]

This null check is redundant, because null is never instanceof anything.

[smellsblue]

👍

[cky]

This null check is redundant, because null is never instanceof anything.

[smellsblue]

👍

[cky]

mapToLong(JavaThingView::getInstancesCountWithoutSubclasses)

[cky]

Wouldn't you want to compress the two transform operations into a single one?

private ReferrerSet getReferrersInternal() {
    return new ReferrerSet(this, Lists.transform(params.get("referrer"),
            referrer -> new JavaThingView(this, resolveClass(referrer, false))));
}

[cky]

For consistency for all the other places in the code that specify a UTF-8 encoding, this should also include one.

[cky]

I'm hoping that most browsers support CDATA sections (<![CDATA[...]]>) by now. If that's the case, you should be able to wrap a PrintWriter over out directly and skip the StringWriter, with the understanding that stack traces do not (or ought not) contain the ]]> sequence.

[cky]

Wow. That's nothing short of beautiful. Yay for zero lines of code!

[cky]

Here's the next batch of review comments!

[cky]

I would stash snapshot.getOqlEngine()'s result into a variable, since getOqlEngine() accesses a ThreadLocal each call. That adds up to something painful.

(Yes, this comment applies to the previous version of the code too. So this isn't a regression or anything. But it should still be fixed.)

[cky]

This kind of stuff is why I like C# better. In C#, you'd write virtual on the method to specify that it should be overridable, which gives the reader instant feedback that it's, well, designed to be overridden, rather than be just an inane one-liner that returns "200 OK". ;-)

[cky]

In Guava, using new ImmutableList.Builder<String>() is preferable to ImmutableList.<String>builder() when the type parameter cannot be correctly inferred.

Also, semantically, I'd have preferred ImmutableMultimap for this, but that may be YAGNI for now.

[cky]

We should probably think about creating some kind of getBoolean method for boolean parameters, returning true for true, yes, or 1, returning false for false, no, or 0, and throwing an exception otherwise.

[cky]

Consider using:

return buildParams().entries().stream()
        .map(entry -> String.format("%s=%s", Misc.encodeForURL(entry.getKey()), Misc.encodeForURL(entry.getValue())))
        .collect(Collectors.joining("&"));

[cky]

Guava memoising supplier, etc.

[cky]

I would inline the definition of newJavaThingView into this.

[cky]

This should be an inner class.

public class WithValue extends ViewModel {
    private final JavaThingView value;

    public WithValue(QueryHandler handler, JavaThingView value) {
        super(handler);
        this.value = value;
    }

    public JavaFieldView getField() {
        return JavaFieldView.this;
    }

    public JavaThingView getValue() {
        return value;
    }
}

[cky]

Mutating JavaPackage instances like that is too gross for my liking. :-( Here's a proposed alternative implementation:

public class JavaPackage extends ViewModel {
    private final String name;
    private final List<JavaThingView> classes;

    public JavaPackage(QueryHandler handler, String name, Collection<JavaThingView> classes) {
        super(handler);
        this.name = name;
        this.classes = ImmutableList.copyOf(classes);
    }

    public static List<JavaPackage> getAll(QueryHandler handler, boolean excludePlatformClasses) {
        Stream<JavaClass> stream = handler.getSnapshot().getClasses().stream();
        if (excludePlatformClasses) {
            stream = stream.filter(x -> !PlatformClasses.isPlatformClass(x));
        }
        return stream.map(clazz -> new JavaThingView(handler, clazz))
                .collect(Collectors.groupingBy(JavaThingView::getPackageName))
                .entrySet().stream()
                .map(entry -> new JavaPackage(handler, entry.getKey(), entry.getValue()))
                .collect(Collectors.toList());
    }

    public String getName() {
        return name;
    }

    public List<JavaThingView> getClasses() {
        return classes;
    }
}

You can micro-optimise this further, by passing a toImmutableList collector as the second argument of groupingBy, thus removing the need for ImmutableList.copyOf:

public class JavaPackage extends ViewModel {
    private final String name;
    private final ImmutableList<JavaThingView> classes;

    public JavaPackage(QueryHandler handler, String name, ImmutableList<JavaThingView> classes) {
        super(handler);
        this.name = name;
        this.classes = classes;
    }

    public static List<JavaPackage> getAll(QueryHandler handler, boolean excludePlatformClasses) {
        Stream<JavaClass> stream = handler.getSnapshot().getClasses().stream();
        if (excludePlatformClasses) {
            stream = stream.filter(x -> !PlatformClasses.isPlatformClass(x));
        }
        return stream.map(clazz -> new JavaThingView(handler, clazz))
                .collect(Collectors.groupingBy(JavaThingView::getPackageName, ImmutableList.toImmutableList()))
                .entrySet().stream()
                .map(entry -> new JavaPackage(handler, entry.getKey(), entry.getValue()))
                .collect(Collectors.toList());
    }

    // ...
}

Note that ImmutableList.toImmutableList() is new to Guava 21. In older Guava versions, here's a polyfill you can use:

public static <T> Collector<T, ImmutableList.Builder<T>, ImmutableList<T>> toImmutableList() {
    return Collector.of(ImmutableList::builder, ImmutableList.Builder::add,
            (lhs, rhs) -> lhs.addAll(rhs.build()), ImmutableList.Builder::build);
}

[cky]

There is a simpler algorithm for printing human-readable byte counts. In fact, I wrote some sample code for a Stack Overflow answer once: http://stackoverflow.com/a/2510468/13

Converted to Java, it'd look like:

// Too bad Java doesn't have Math.log2(); then LOG2_1024 is simply 10.
private static final double LOG_1024 = Math.log(1024);
private static final String[] SUFFIXES = {"B", "kB", "MB", "GB", "TB"};

private static String formatHumanReadableSize(double size) {
    double base = Math.log(size) / LOG_1024;
    return String.format("%.2f%s", Math.pow(1024, base % 1.0), SUFFIXES[(int) Math.floor(base)]);
}

[cky]

Okay, I've finally finished the review! Have at it. :-)

Note that I did not review the Mustache templates, as they don't really contain code, and I trust that any bugs there would be immediately visible and remediable.

[cky]

This should not need caching. Or, if caching is desired, it should be done at the model level.

[cky]

I know the "string literal".equals(someString) pattern is used a lot in our work code, because our work code uses nulls excessively much, but in this code base, I don't believe we should use this pattern. Nulls should be the exception rather than the rule, and blowing up on null is not a bad thing in that case.

It might also be a good idea to have a set of keys we like "expanded" by default, for ease of extensibility:

private static final Set<String> EXPANDED_KEYS = ImmutableSet.of("@attributes");

// ...
if (EXPANDED_KEYS.contains(entry.getKey())) { ... }

[cky]

Use Arrays.stream(...) instead of Arrays.asList(...).stream().

[cky]

Consider storing the Integer and the Collection<ReferenceChain> as separate fields, rather than holding onto the Map.Entry object (which may hold a reference to the backing map and block it from being GC'd).

[cky]

You may find this easier to write using Guava's AbstractIterator:

public class ReferenceChainIterator extends AbstractIterator<ReferenceChainView> {
    private ReferenceChain iter = reference;

    @Override
    protected ReferenceChainView computeNext() {
        if (iter == null) {
            return endOfData();
        }
        ReferenceChainView result = new ReferenceChainView(handler, iter);
        iter = iter.getNext();
        return result;
    }
}

[cky]

Consider storing the Integer and the Collection<Root> as separate fields, rather than holding onto the Map.Entry object (which may hold a reference to the backing map and block it from being GC'd).

[cky]

I would inline newRootView directly in here, unless you have a good reason not to do that.

[cky]

I remain of the opinion that this class is a hack and should be replaced in all cases with collect(Collectors.toList()) or something similar. However, if you do decide to keep this class, there should be check that iterator() is not called more than once, also. The check need not be in this method; it could, for example, be in StreamIterator's constructor if you prefer.

[cky]

I've finished reviewing your newest batch of commits. Keep sending changes through! :-)

[cky]

You can also use printf("Dump file created %tc%n", in.readLong()), or customise the date format however you like. This way you don't need to create a Date object.

[cky]

Use %#x instead of 0x%x.

[cky]

Ditto.

[cky]

"test".equals(scope) is mildly deprecated, because it's hard for humans to decipher the intent. I tend to prefer to signal my intent directly:

1. If scope can contain null as a valid value, use Objects.equals(scope, "test").
2. If null is not a valid value, just use scope.equals("test").

[cky]

Very nice catch. :-)

[cky]

I'm not a lawyer, but, I don't believe you need this part of the preamble for any original code we write.

[cky]

I would DRY this further:

String resourcePath = String.format("/com/sun/tools/hat/resources/%s.mustache", resourceName);
InputStream stream = MustacheQueryHandler.class.getResourceAsStream(resourcePath);
Preconditions.checkArgument(stream != null, "Invalid resource: %s", resourcePath);
return new InputStreamReader(stream, Charsets.UTF_8);

Two notes:

1. I use IllegalArgumentException instead of IllegalStateException because the error is caused by an invalid value for resourceName. However, that's just my view, and if you feel strongly about using IllegalStateException, I won't block you.
2. Note the explicit specification of UTF-8 encoding. We should in fact vet all usages of InputStreamReader and such like to ensure that the encoding is explicitly specified, always. This is also a personal opinion; it reflects my view that, in any context where it's not user-specified what the encoding is (i.e., we're not dealing with user inpuit of some kind), we should use UTF-8 across the board.

[cky]

My comment to apply synchronized stuff isn't meant in quite this way. ;-) What you've got here is pretty much no different from using the volatile as before. The idea behind the synchronized is to make the atomic units of work a little more coarse-grained, so that the snapshot and loadingSnapshot variables are altered in lock step. This will require some minor restructuring of the code to make work.

Of course, I believe you made some comment about this code being only ever called from the main thread. If that's the case, just document that, and forget about any locking.