Skip to content

Conversation

@HTHou
Copy link
Contributor

@HTHou HTHou commented Jul 6, 2021

No description provided.

}

public void updateStartTime(String device, long time) {
device = device.intern();
Copy link
Member

Choose a reason for hiding this comment

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

LGTM.
hi , haonan , I have a minor question.
Because using the internal method has a performance impact. If intern() is performed after a new string is generated, the query performance will be affected,This needs to be considered. If the memory consumption and gc time are longer than intern, I agree with using intern().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, this pr is under testing and won't be merged until we get the performance result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, it is a temporary way to reduce the memory size of TsFileResource. Actually, we are considering to remove the Devices set in FileTimeIndex.

Copy link
Member

Choose a reason for hiding this comment

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

ok. no problem..

@HTHou HTHou changed the title Use String.intern() in TsFileResource Use String.intern() in TsFileResource to reduce the memory size Jul 6, 2021
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 7, 2021

@HTHou HTHou changed the title Use String.intern() in TsFileResource to reduce the memory size [To rel/0.12] Use StringCachedPool in TsFileResource to reduce the memory size Jul 7, 2021
@SteveYurongSu SteveYurongSu merged commit 36ae1e5 into rel/0.12 Jul 7, 2021
@SteveYurongSu SteveYurongSu deleted the ResourceIntern12 branch July 7, 2021 07:57
HTHou added a commit that referenced this pull request Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants