Skip to content

Commit

Permalink
8286740: JFR: Active Setting event emitted incorrectly
Browse files Browse the repository at this point in the history
Reviewed-by: jbachorik
Backport-of: 24cab0af32a1eaa4c594fb2a144386a6b7062981
  • Loading branch information
richardstartin authored and Jaroslav Bachorik committed Mar 14, 2024
1 parent c9b0307 commit 4b9ab0d
Show file tree
Hide file tree
Showing 7 changed files with 56 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,6 @@
@StackTrace(false)
public final class ActiveSettingEvent extends AbstractJDKEvent {

public static final ThreadLocal<ActiveSettingEvent> EVENT = new ThreadLocal<ActiveSettingEvent>() {
@Override
protected ActiveSettingEvent initialValue() {
return new ActiveSettingEvent();
}
};

@Label("Event Id")
public long id;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -290,13 +290,13 @@ void writeActiveSettingEvent() {
if (!type.isRegistered()) {
return;
}
ActiveSettingEvent event = ActiveSettingEvent.EVENT.get();
for (NamedControl nc : namedControls) {
if (Utils.isSettingVisible(nc.control, type.hasEventHook())) {
String value = nc.control.getLastValue();
if (value == null) {
value = nc.control.getDefaultValue();
}
ActiveSettingEvent event = new ActiveSettingEvent();
event.id = type.getId();
event.name = nc.name;
event.value = value;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ public synchronized EventType register(Class<? extends jdk.internal.event.Event>
handler.setRegistered(true);
typeLibrary.addType(handler.getPlatformEventType());
if (jvm.isRecording()) {
settingsManager.setEventControl(handler.getEventControl());
settingsManager.setEventControl(handler.getEventControl(), true);
settingsManager.updateRetransform(Collections.singletonList((eventClass)));
}
setStaleMetadata();
Expand Down Expand Up @@ -199,8 +199,8 @@ private EventHandler makeHandler(Class<? extends jdk.internal.event.Event> event
}


public synchronized void setSettings(List<Map<String, String>> list) {
settingsManager.setSettings(list);
public synchronized void setSettings(List<Map<String, String>> list, boolean writeSettingEvents) {
settingsManager.setSettings(list, writeSettingEvents);
}

synchronized void disableEvents() {
Expand Down
14 changes: 7 additions & 7 deletions src/jdk.jfr/share/classes/jdk/jfr/internal/PlatformRecorder.java
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ synchronized long start(PlatformRecording recording) {
currentChunk.setStartTime(startTime);
}
recording.setState(RecordingState.RUNNING);
updateSettings();
updateSettings(false);
recording.setStartTime(startTime);
writeMetaEvents();
} else {
Expand All @@ -275,7 +275,7 @@ synchronized long start(PlatformRecording recording) {
startTime = Utils.epochNanosToInstant(startNanos);
recording.setStartTime(startTime);
recording.setState(RecordingState.RUNNING);
updateSettings();
updateSettings(false);
writeMetaEvents();
if (currentChunk != null) {
finishChunk(currentChunk, startTime, recording);
Expand Down Expand Up @@ -339,7 +339,7 @@ synchronized void stop(PlatformRecording recording) {
} else {
RepositoryChunk newChunk = null;
RequestEngine.doChunkEnd();
updateSettingsButIgnoreRecording(recording);
updateSettingsButIgnoreRecording(recording, false);

String path = null;
if (toDisk) {
Expand Down Expand Up @@ -383,19 +383,19 @@ private void disableEvents() {
MetadataRepository.getInstance().disableEvents();
}

void updateSettings() {
updateSettingsButIgnoreRecording(null);
void updateSettings(boolean writeSettingEvents) {
updateSettingsButIgnoreRecording(null, writeSettingEvents);
}

void updateSettingsButIgnoreRecording(PlatformRecording ignoreMe) {
void updateSettingsButIgnoreRecording(PlatformRecording ignoreMe, boolean writeSettingEvents) {
List<PlatformRecording> recordings = getRunningRecordings();
List<Map<String, String>> list = new ArrayList<>(recordings.size());
for (PlatformRecording r : recordings) {
if (r != ignoreMe) {
list.add(r.getSettings());
}
}
MetadataRepository.getInstance().setSettings(list);
MetadataRepository.getInstance().setSettings(list, writeSettingEvents);
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,7 @@ public void setSetting(String id, String value) {
synchronized (recorder) {
this.settings.put(id, value);
if (getState() == RecordingState.RUNNING) {
recorder.updateSettings();
recorder.updateSettings(true);
}
}
}
Expand All @@ -489,7 +489,7 @@ private void setSettings(Map<String, String> settings, boolean update) {
synchronized (recorder) {
this.settings = new LinkedHashMap<>(settings);
if (getState() == RecordingState.RUNNING && update) {
recorder.updateSettings();
recorder.updateSettings(true);
}
}
}
Expand Down
10 changes: 6 additions & 4 deletions src/jdk.jfr/share/classes/jdk/jfr/internal/SettingsManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ public void finish() {

private Map<String, InternalSetting> availableSettings = new LinkedHashMap<>();

void setSettings(List<Map<String, String>> activeSettings) {
void setSettings(List<Map<String, String>> activeSettings, boolean writeSettingEvents) {
// store settings so they are available if a new event class is loaded
availableSettings = createSettingsMap(activeSettings);
List<EventControl> eventControls = MetadataRepository.getInstance().getEventControls();
Expand All @@ -143,7 +143,7 @@ void setSettings(List<Map<String, String>> activeSettings) {
Collections.sort(eventControls, (x,y) -> x.getEventType().getName().compareTo(y.getEventType().getName()));
}
for (EventControl ec : eventControls) {
setEventControl(ec);
setEventControl(ec, writeSettingEvents);
}
}
if (JVM.getJVM().getAllowedToDoEventRetransforms()) {
Expand Down Expand Up @@ -211,7 +211,7 @@ private Collection<InternalSetting> makeInternalSettings(Map<String, String> rec
return internals.values();
}

void setEventControl(EventControl ec) {
void setEventControl(EventControl ec, boolean writeSettingEvents) {
InternalSetting is = getInternalSetting(ec);
boolean shouldLog = Logger.shouldLog(LogTag.JFR_SETTING, LogLevel.INFO);
if (shouldLog) {
Expand Down Expand Up @@ -250,7 +250,9 @@ void setEventControl(EventControl ec) {
}
}
}
ec.writeActiveSettingEvent();
if (writeSettingEvents) {
ec.writeActiveSettingEvent();
}
if (shouldLog) {
Logger.log(LogTag.JFR_SETTING, LogLevel.INFO, "}");
}
Expand Down
38 changes: 37 additions & 1 deletion test/jdk/jdk/jfr/event/runtime/TestActiveSettingEvent.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2017, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2017, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand All @@ -23,8 +23,10 @@
package jdk.jfr.event.runtime;

import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

import jdk.jfr.Configuration;
import jdk.jfr.Event;
Expand Down Expand Up @@ -60,6 +62,7 @@ private static class MyRegistrationEvent extends Event {
public static void main(String[] args) throws Throwable {
testDefaultSettings();;
testProfileSettings();;
testOnlyOnce();
testNewSettings();
testChangedSetting();
testUnregistered();
Expand All @@ -74,6 +77,39 @@ private static void testDefaultSettings() throws Exception {
testSettingConfiguration("default");
}

private static void testOnlyOnce() throws Exception {
Configuration c = Configuration.getConfiguration("default");
try (Recording r = new Recording(c)) {
r.enable(ACTIVE_SETTING_EVENT_NAME).withStackTrace();
r.start();
r.stop();
Map<String, RecordedEvent> settings = new HashMap<>();
List<RecordedEvent> events = Events.fromRecording(r);
for (RecordedEvent e : events) {
if (e.getEventType().getName().equals(ACTIVE_SETTING_EVENT_NAME)) {
long id = e.getLong("id");
String name = e.getString("name");
String value = e.getString("value");
String s = id + "#" + name + "=" + value;
if (settings.containsKey(s)) {
System.out.println("Event:");
System.out.println(settings.get(s));
System.out.println("Duplicated by:");
System.out.println(e);
String message = "Found duplicated setting '" + s + "'";
for (EventType type : FlightRecorder.getFlightRecorder().getEventTypes()) {
if (type.getId() == id) {
throw new Exception(message+ " for " + type.getName());
}
}
throw new Exception(message);
}
settings.put(s, e);
}
}
}
}

private static void testRegistration() throws Exception {
// Register new
try (Recording recording = new Recording()) {
Expand Down

1 comment on commit 4b9ab0d

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

Please sign in to comment.