Skip to content

Commit

Permalink
Fix memory leak large ReadOnlyStyledDocument models through MappingGe…
Browse files Browse the repository at this point in the history
…neratorPane
  • Loading branch information
Col-E committed Jun 3, 2024
1 parent 79ad5cb commit f1da92e
Show file tree
Hide file tree
Showing 7 changed files with 72 additions and 45 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,15 @@ public static DebuggingLogger get(Class<?> cls) {
* @param consumer
* New log message consumer.
*/
public static void addLogConsumer(LogConsumer<String> consumer) {
public static void addLogConsumer(@Nonnull LogConsumer<String> consumer) {
logConsumers.add(consumer);
}

/**
* @param consumer
* Log message consumer to remove.
*/
public static void removeLogConsumer(LogConsumer<String> consumer) {
public static void removeLogConsumer(@Nonnull LogConsumer<String> consumer) {
logConsumers.remove(consumer);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,12 @@ public boolean summarize(@Nonnull Workspace workspace,
Stage window = windowFactory.createAnonymousStage(scene, getBinding("mapgen"), 800, 400);
window.show();
window.requestFocus();

// Because our service is application scoped, the injected mapping generator panes won't
// be automatically destroyed until all of Recaf is closed. Thus, for optimal GC usage we
// need to manually invoke the destruction of our injected mapping generator panes.
// We can do this when the stage is closed.
window.setOnHidden(e -> generatorPaneProvider.destroy(mappingGeneratorPane));
});
}, service).exceptionally(t -> {
logger.error("Failed to open mapping viewer", t);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1923,7 +1923,7 @@ private <T extends AbstractSearchPane> T openSearchPane(@Nonnull String titleId,
region = dockingManager.newRegion();
DockingTab tab = region.createTab(getBinding(titleId), content);
tab.setGraphic(new FontIconView(icon));
RecafScene scene = new RecafScene((region));
RecafScene scene = new RecafScene(region);
Stage window = windowFactory.createAnonymousStage(scene, getBinding("menu.search"), 800, 400);
window.show();
window.requestFocus();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import javafx.stage.Stage;
import software.coley.recaf.services.Service;
import software.coley.recaf.ui.window.RecafStage;
import software.coley.recaf.util.Icons;

/**
* Creates new windows.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import jakarta.enterprise.context.ApplicationScoped;
import jakarta.enterprise.inject.Instance;
import jakarta.inject.Inject;
import javafx.event.EventHandler;
import javafx.stage.Stage;
import javafx.stage.WindowEvent;
import org.slf4j.Logger;
Expand All @@ -27,6 +26,7 @@
public class WindowManager implements Service {
private static final Logger logger = Logging.get(WindowManager.class);
public static final String SERVICE_ID = "window-manager";
private static final String ANON_PREFIX = "anon-";
// Built-in window keys
public static final String WIN_MAIN = "main";
public static final String WIN_REMOTE_VMS = "remote-vms";
Expand All @@ -41,7 +41,7 @@ public class WindowManager implements Service {
private final Map<String, Stage> windowMappings = new HashMap<>();

@Inject
public WindowManager(WindowManagerConfig config, Instance<IdentifiableStage> stages) {
public WindowManager(@Nonnull WindowManagerConfig config, @Nonnull Instance<IdentifiableStage> stages) {
this.config = config;

// Register identifiable stages.
Expand All @@ -58,7 +58,7 @@ public WindowManager(WindowManagerConfig config, Instance<IdentifiableStage> sta
* Stage to register.
*/
public void registerAnonymous(@Nonnull Stage stage) {
register(UUID.randomUUID().toString(), stage);
register(ANON_PREFIX + UUID.randomUUID(), stage);
}

/**
Expand All @@ -84,24 +84,24 @@ public void register(@Nonnull String id, @Nonnull Stage stage) {
if (windowMappings.containsKey(id))
throw new IllegalStateException("The stage ID was already registered: " + id);

EventHandler<WindowEvent> baseOnShown = stage.getOnShown();
EventHandler<WindowEvent> baseOnHidden = stage.getOnHidden();

// Wrap original handlers to keep existing behavior.
// Record when windows are 'active' based on visibility.
EventHandler<WindowEvent> onShown = e -> {
// We're using event filters so users can still do things like 'stage.setOnShown(...)' and not interfere with
// our window tracking logic in here
stage.addEventFilter(WindowEvent.WINDOW_SHOWN, e -> {
logger.trace("Stage showing: {}", id);
activeWindows.add(stage);
if (baseOnShown != null) baseOnShown.handle(e);

};
EventHandler<WindowEvent> onHidden = e -> {
});
stage.addEventFilter(WindowEvent.WINDOW_HIDDEN, e -> {
logger.trace("Stage hiding: {}", id);
activeWindows.remove(stage);
if (baseOnHidden != null) baseOnHidden.handle(e);
};
stage.setOnShown(onShown);
stage.setOnHidden(onHidden);

// Anonymous stages can be pruned from the id->stage map.
// They are not meant to be persistent. But, we register them anyway for our duplicate register check above.
if (id.startsWith(ANON_PREFIX)) {
logger.trace("Stage pruned: {} ({})", id, stage.getTitle());
windowMappings.remove(id);
}
});

// If state is already visible, add it right away.
if (stage.isShowing()) activeWindows.add(stage);
Expand All @@ -119,7 +119,7 @@ public void register(@Nonnull String id, @Nonnull Stage stage) {
* @return Active windows.
*/
@Nonnull
public ObservableList<Stage> getActiveWindows() {
public Collection<Stage> getActiveWindows() {
return activeWindows;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ public CompletableFuture<Void> restyleAtPosition(int position, int length) {
*
* @return The {@link StackPane} present in the {@link #getCenter() center} of the editor.
*/
@Nonnull
public StackPane getPrimaryStack() {
return stackPane;
}
Expand All @@ -216,6 +217,7 @@ public void redrawParagraphGraphics() {
/**
* @return Current style spans for the entire document.
*/
@Nonnull
public StyleSpans<Collection<String>> getStyleSpans() {
return codeArea.getStyleSpans(0, getTextLength());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import atlantafx.base.theme.Styles;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import jakarta.annotation.PreDestroy;
import jakarta.enterprise.context.Dependent;
import jakarta.enterprise.inject.Instance;
import jakarta.inject.Inject;
Expand Down Expand Up @@ -85,18 +86,18 @@ public class MappingGeneratorPane extends StackPane {
private final InheritanceGraph graph;
private final ModalPane modal = new ModalPane();
private final MappingApplier mappingApplier;
private final Node previewGroup;
private final Pane previewGroup;

@Inject
public MappingGeneratorPane(@Nonnull Workspace workspace,
@Nonnull NameGeneratorProviders nameGeneratorProviders,
@Nonnull StringPredicateProvider stringPredicateProvider,
@Nonnull MappingGenerator mappingGenerator,
@Nonnull ConfigComponentManager componentManager,
@Nonnull InheritanceGraph graph,
@Nonnull AggregateMappingManager aggregateMappingManager,
@Nonnull MappingApplier mappingApplier,
@Nonnull Instance<SearchBar> searchBarProvider) {
@Nonnull NameGeneratorProviders nameGeneratorProviders,
@Nonnull StringPredicateProvider stringPredicateProvider,
@Nonnull MappingGenerator mappingGenerator,
@Nonnull ConfigComponentManager componentManager,
@Nonnull InheritanceGraph graph,
@Nonnull AggregateMappingManager aggregateMappingManager,
@Nonnull MappingApplier mappingApplier,
@Nonnull Instance<SearchBar> searchBarProvider) {

this.workspace = workspace;
this.nameGeneratorProviders = nameGeneratorProviders;
Expand Down Expand Up @@ -126,6 +127,16 @@ public MappingGeneratorPane(@Nonnull Workspace workspace,
getChildren().addAll(modal, horizontalWrapper);
}

@PreDestroy
private void destroy() {
// We want to clear out a number of things here to assist in proper GC cleanup.
// - Mappings (which can hold a workspace/graph reference)
// - UI components (which can hold large virtualized text models for the mapping text representation)
mappingsToApply.setValue(null);
previewGroup.getChildren().clear();
getChildren().clear();
}

public void addConfiguredFilter(@Nonnull FilterWithConfigNode<?> filterConfig) {
filters.getItems().add(filterConfig);
}
Expand Down Expand Up @@ -212,6 +223,9 @@ private void apply() {
if (mappings != null) {
MappingResults results = mappingApplier.applyToPrimaryResource(mappings);
results.apply();

// Clear property now that the mappings have been applied
mappingsToApply.set(null);
}
}

Expand Down Expand Up @@ -293,18 +307,7 @@ public String fromString(String s) {
@Nonnull
private Node createFilterDisplay(@Nonnull AggregateMappingManager aggregateMappingManager) {
// List to house current filters.
filters.setCellFactory(param -> new ListCell<>() {
@Override
protected void updateItem(FilterWithConfigNode<?> item, boolean empty) {
super.updateItem(item, empty);
if (empty || item == null) {
textProperty().unbind();
setText(null);
} else {
textProperty().bind(item.display().map(display -> (getIndex() + 1) + ". " + display));
}
}
});
filters.setCellFactory(param -> new ConfiguredFilterCell());
filters.getItems().add(new ExcludeExistingMapped(aggregateMappingManager.getAggregatedMappings()));
ReadOnlyObjectProperty<FilterWithConfigNode<?>> selectedItem = filters.getSelectionModel().selectedItemProperty();
BooleanBinding hasItemSelection = selectedItem.isNull();
Expand Down Expand Up @@ -370,9 +373,9 @@ protected void updateItem(FilterWithConfigNode<?> item, boolean empty) {

@Nonnull
private ActionMenuItem typeSetAction(@Nonnull ObjectProperty<Supplier<FilterWithConfigNode<?>>> nodeSupplier,
@Nonnull StringProperty parentText,
@Nonnull String translationKey,
@Nonnull Supplier<FilterWithConfigNode<?>> supplier) {
@Nonnull StringProperty parentText,
@Nonnull String translationKey,
@Nonnull Supplier<FilterWithConfigNode<?>> supplier) {
StringBinding nameBinding = Lang.getBinding(translationKey);
return new ActionMenuItem(nameBinding,
() -> {
Expand Down Expand Up @@ -781,6 +784,23 @@ protected void fillConfigurator(@Nonnull BiConsumer<StringBinding, Node> sink) {
}
}

/**
* List cell to render {@link FilterWithConfigNode}.
*/
private static class ConfiguredFilterCell extends ListCell<FilterWithConfigNode<?>> {
@Override
protected void updateItem(FilterWithConfigNode<?> item, boolean empty) {
super.updateItem(item, empty);
StringProperty property = textProperty();
if (empty || item == null) {
property.unbind();
setText(null);
} else {
property.bind(item.display().map(display -> (getIndex() + 1) + ". " + display));
}
}
}

/**
* Base to create a filter with configuration node.
*
Expand Down

0 comments on commit f1da92e

Please sign in to comment.