Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Refresh LogstashBasicMarker implementation and remove synchronisation #617

Merged
merged 4 commits into from
Aug 24, 2021

Conversation

brenuart
Copy link
Collaborator

fixes #613

Additional references can be added to a Marker after it is created. Including the references in the equals/hashCode methods leads to unstable results.
This change revert to the standard Marker equals/hashCode behaviour as defined by SLF4J BasicMarker implementation.

Closes #616
@brenuart
Copy link
Collaborator Author

Also fixes #616

@brenuart brenuart marked this pull request as ready for review August 24, 2021 11:11
@brenuart brenuart linked an issue Aug 24, 2021 that may be closed by this pull request
@brenuart brenuart merged commit 432a427 into main Aug 24, 2021
@brenuart brenuart deleted the gh613-basic-marker branch August 24, 2021 11:14
private final String name;
private List<Marker> refereceList;

private final List<Marker> referenceList = new CopyOnWriteArrayList<>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I realize this is coming from slf4j-api's implementation, but this seems like a step in the wrong direction, since it will result in a lot of unused lists being allocated.

If markers and/or structured args are used heavily by an application, this will have an impact on memory usage and garbage collection.

The previous logic of deferring list creation until a reference is added seems better from a memory and garbage collection standpoint.

Having said that, I don't have exact numbers to back these claims up, so probably better to go with slf4j-api's implementation until proven otherwise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree with you... creating the list lazily would be better. However, this makes supporting concurrent accesses without synchronisation far less easier...

AFAIK people are not supposed to create many markers - hence the MarkerFactory caching previously created markers. Is it still true with Logstash?

Copy link
Collaborator

Choose a reason for hiding this comment

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

LogstashBasicMarker is the superclass of all of the markers and structured argument implementations provided by logstash-logback-encoder. Caching is not used for any of these. So, for example, a marker (with an unused list) will be created every time this log line is executed.

logger.info("Foo {}", StructuredArguments.keyValue("bar", "baz"));

If that line is executed frequently, it'll create a lot of unused list garbage.

slf4j-api can cache the markers it creates, since those markers are simple string values, so they are easily cachable.

In any case, I understand the synchronization problems being addressed, so we can leave it as is. It's prioritizing performance over memory usage. Classic tradeoff.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I understand the synchronization problems being addressed, so we can leave it as is. It's prioritizing performance over memory usage

But I'm also fighting against excessive memory allocations ;-)
The key point is whether LogstashBasicMarker must be thread-safe or not... IMHO, markers are not shared/mutated by multiple threads in most scenarios.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@philsttr I'm tempted to "drop" the thread-safe nature of the marker... certainly if we consider what has been said in #613 (comment)

Copy link
Collaborator Author

@brenuart brenuart Sep 6, 2021

Choose a reason for hiding this comment

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

@philsttr Back on the referenceList again... The overhead of creating the COW list up-front is:

  • a zero-size object array
  • a ReentrantLock

Not much - but still too much if we consider that most LogstashMarkers won't have any children/references. BTW, what would be the use case? Can't we simply drop support for references and make the relevant methods "not implemented" ?

Alternatives:

(1) make it 'not thread-safe'. Start with a null reference list and lazy initialise it with a standard ArrayList when needed. Behaviour becomes unexpected if the Marker is accessed from multiple threads concurrently. Note that a COW list won't probably make multi-threaded usage more predictable: a first call to hasReferences() may return true but the list may be empty when you start iterating over it if another thread removed the elements in the meantime.

(2) keep it "thread-safe" but use an AtomicReference to hold the COW list. The overhead when no reference is ever added to the marker is limited to this atomic reference which is cheaper than the empty COW itself.

What's your opinion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW, what would be the use case? Can't we simply drop support for references and make the relevant methods "not implemented" ?

Unfortunately not, since the slf4j-api Logger methods only accept a single Marker argument. Support for multiple markers is implemented by having markers reference others.

Regarding thread-safety. I would rather keep it threadsafe. Another option (3).... similar to (2) but instead of using an AtomicReference, we could use a volatile field accessed via a static AtomicReferenceFieldUpdater. This prevents memory overhead of creating a lot of AtomicReference instances, at the expense of slightly slower access time and slightly more complex code.

@philsttr philsttr added this to the 7.0 milestone Aug 28, 2021
@philsttr philsttr added the warn/api-change Breaking change with compilation or xml configuration errors label Aug 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
warn/api-change Breaking change with compilation or xml configuration errors
Projects
None yet
2 participants