You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2022/01/24 03:53:01 UTC
[GitHub] [iotdb] chengjianyun opened a new pull request #4964: Apply string intern for device String
chengjianyun opened a new pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964
The implementation of cacheString pool has the memory leak risk. We can use string intern instead of do it by ourself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] Alima777 edited a comment on pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
Alima777 edited a comment on pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#issuecomment-1023035396
> BTW, memory leak is intolerable in any cases especially in a database server system in my opinion.
Hi, I understand your concerns. So what I said is `we can delete useless string from cachedStringPool manually when time series is deleted` to avoid memory leak. I didn't check the conclusion about memory footprint using `WeakHashMap` is 4~5 times than `intern()`, is `concurrentHashMap` still the same?
And `-XX:StringTableSize` still need to be set based on the load of each user, it's not flexible anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] wangchao316 commented on a change in pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
wangchao316 commented on a change in pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#discussion_r790525012
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/VirtualStorageGroupProcessor.java
##########
@@ -45,14 +45,7 @@
import org.apache.iotdb.db.engine.upgrade.UpgradeLog;
import org.apache.iotdb.db.engine.version.SimpleFileVersionController;
import org.apache.iotdb.db.engine.version.VersionController;
-import org.apache.iotdb.db.exception.BatchProcessException;
-import org.apache.iotdb.db.exception.DiskSpaceInsufficientException;
-import org.apache.iotdb.db.exception.LoadFileException;
-import org.apache.iotdb.db.exception.StorageGroupProcessorException;
-import org.apache.iotdb.db.exception.TriggerExecutionException;
-import org.apache.iotdb.db.exception.TsFileProcessorException;
-import org.apache.iotdb.db.exception.WriteProcessException;
-import org.apache.iotdb.db.exception.WriteProcessRejectException;
+import org.apache.iotdb.db.exception.*;
Review comment:
the same as above...
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/TsFileResource.java
##########
@@ -54,13 +54,7 @@
import java.nio.file.FileAlreadyExistsException;
import java.nio.file.Files;
import java.nio.file.Paths;
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
-import java.util.Random;
-import java.util.Set;
+import java.util.*;
Review comment:
Good
does not import *
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] coveralls edited a comment on pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#issuecomment-1019717716
[](https://coveralls.io/builds/45908765)
Coverage decreased (-0.06%) to 67.672% when pulling **516f46c88583912d7cd525f619a4639281df6451 on chengjianyun:jianyun/intern** into **4e87d43d0bd18b30f96db9745bdef89c6ee1f4c1 on apache:master**.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] chengjianyun commented on pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
chengjianyun commented on pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#issuecomment-1022975116
> > For example, if we delete some storage groups or timeseries, some string would be never used but they are always kept in `cachedDevicePool` which cause memory leak.
>
> Hi, it's true that we can't handle the useless string now if we delete some devices. But the `intern()` method is not that fast[1][2] since the purpose of it is to save memory. By comparison, `concurrentHashMap` has a good performance. I believe that the situation about deleting timeseries or storage group is relatively few. And we use `deviceIndexMap` when we initialize TsFileResource, when restarting or ingesting data, the performance seems more important. Maybe we need to make a balance here. Or we can delete useless string from `cachedStringPool` manually when time series is deleted instead of just using `intern()`. What do you think?
>
> [1] https://stackoverflow.com/questions/56203500/java-string-intern-use-hashtable-instead-of-concurrenthashmap [2] https://www.fatalerrors.org/a/jvm-series-string.intern-performance-of.html
@Alima777, about performance of `String.intern()`, you can also refer [this blog](http://java-performance.info/string-intern-in-java-6-7-8/). I have verified the conclusion and got similar result on JDK8. So, it's better to set the `-XX:StringTableSize` when go bad perfromance on `intern()` method. And the performance improved much since JDK11 event without `-XX:StringTableSize` parameter.
BTW, memory leak is intolerable in any cases especially in a database server system.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] chengjianyun commented on pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
chengjianyun commented on pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#issuecomment-1023097165
> we can delete useless string from cachedStringPool manually when time series is deleted
As far as I know, we can do this by count references to each object like older verion JVM GC does. If we do like this, why don't let JVM help recycle the memory usage by use `WeakHashMap`? Or do you have other ideas?
> `WeakHashMap` is 4~5 times than`intern()`, is `concurrentHashMap` still the same?
The HashMap or ConcurrentHashMap implemented CachePool is wrong without explicit clear, so I didn't verify with them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] Alima777 edited a comment on pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
Alima777 edited a comment on pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#issuecomment-1023035396
> BTW, memory leak is intolerable in any cases especially in a database server system in my opinion.
Hi, I understand your concerns. So what I said is `we can delete useless string from cachedStringPool manually when time series is deleted` to avoid memory leak. I didn't check the conclusion about memory footprint using `WeakHashMap` is 4~5 times than `intern()`, is `concurrentHashMap` still the same?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] Alima777 commented on pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
Alima777 commented on pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#issuecomment-1023152367
> As far as I know, we can do this by count references to each object like older verion JVM GC does.
Hi, you should know we only use this `cachedStringPool` in limited situation. It's only used in TsFileResource. So count references maybe not needed, we just add it when initialize, and remove it when delete timeseries.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] JackieTien97 merged pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
JackieTien97 merged pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] Alima777 commented on pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
Alima777 commented on pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#issuecomment-1023035396
> BTW, memory leak is intolerable in any cases especially in a database server system in my opinion.
Hi, I understand your concerns. So what I said is `we can delete useless string from cachedStringPool manually when time series is deleted` to avoid memory leak. I didn't check the conclusion about memory footprint using `WeakHashMap`, maybe it stores more things than normal HashMap?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] HTHou commented on pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
HTHou commented on pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#issuecomment-1021950856
Hi, is there any detailed reason about the memory leak risk?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] Alima777 edited a comment on pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
Alima777 edited a comment on pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#issuecomment-1022885986
> For example, if we delete some storage groups or timeseries, some string would be never used but they are always kept in `cachedDevicePool` which cause memory leak.
Hi, it's true that we can't handle the useless string now if we delete some devices. But the intern() method is not that fast[1][2] since the purpose of it is to save memory. By comparison, `concurrentHashMap` has a good performance.
I believe that the situation about deleting timeseries or storage group is relatively few. And we use `deviceIndexMap` when we initialize TsFileResource, when restarting or ingesting data, the performance seems more important. Maybe we need to make a balance here. Or we can delete useless string from `cachedStringPool` manually when time series is deleted instead of just using intern().
What do you think?
[1] https://stackoverflow.com/questions/56203500/java-string-intern-use-hashtable-instead-of-concurrenthashmap
[2] https://www.fatalerrors.org/a/jvm-series-string.intern-performance-of.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] qiaojialin commented on pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
qiaojialin commented on pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#issuecomment-1023998277
> memory leak is intolerable in any cases especially in a database server system in my opinion.
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] chengjianyun commented on pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
chengjianyun commented on pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#issuecomment-1023932472
> So count references maybe not needed, we just add it when initialize, and remove it when delete timeseries.
I don't think this way would work. As the strings from `CacheStringPool` are passed to different instances, you must know all of the references of a key are released then the `CacheStringPool` could remove the key. Otherwise, you would hold different instances for same string which will lose the meaning of `CacheStringPool`.
And for the class, it designs to provide ability like string intern, but has implementation defects. You can't restrict it only used to hold device or measurement path unless you name it something like `DevicePathPool` or `MeasurementPathPool` instead of `CacheStringPool`.
The two ways I can figure out are either use `WeakHashMap<String, WeakRefernce<String>>` instead of `ConcurrentHashMap` or `String.intern()`. I prefer the later one in the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] chengjianyun commented on a change in pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
chengjianyun commented on a change in pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#discussion_r791320123
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/TsFileResource.java
##########
@@ -54,13 +54,7 @@
import java.nio.file.FileAlreadyExistsException;
import java.nio.file.Files;
import java.nio.file.Paths;
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
-import java.util.Random;
-import java.util.Set;
+import java.util.*;
Review comment:
Done.
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/VirtualStorageGroupProcessor.java
##########
@@ -45,14 +45,7 @@
import org.apache.iotdb.db.engine.upgrade.UpgradeLog;
import org.apache.iotdb.db.engine.version.SimpleFileVersionController;
import org.apache.iotdb.db.engine.version.VersionController;
-import org.apache.iotdb.db.exception.BatchProcessException;
-import org.apache.iotdb.db.exception.DiskSpaceInsufficientException;
-import org.apache.iotdb.db.exception.LoadFileException;
-import org.apache.iotdb.db.exception.StorageGroupProcessorException;
-import org.apache.iotdb.db.exception.TriggerExecutionException;
-import org.apache.iotdb.db.exception.TsFileProcessorException;
-import org.apache.iotdb.db.exception.WriteProcessException;
-import org.apache.iotdb.db.exception.WriteProcessRejectException;
+import org.apache.iotdb.db.exception.*;
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] coveralls edited a comment on pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#issuecomment-1019717716
[](https://coveralls.io/builds/45908765)
Coverage decreased (-0.06%) to 67.672% when pulling **516f46c88583912d7cd525f619a4639281df6451 on chengjianyun:jianyun/intern** into **4e87d43d0bd18b30f96db9745bdef89c6ee1f4c1 on apache:master**.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] Alima777 edited a comment on pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
Alima777 edited a comment on pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#issuecomment-1022885986
> For example, if we delete some storage groups or timeseries, some string would be never used but they are always kept in `cachedDevicePool` which cause memory leak.
Hi, it's true that we can't handle the useless string now if we delete some devices. But the `intern()` method is not that fast[1][2] since the purpose of it is to save memory. By comparison, `concurrentHashMap` has a good performance.
I believe that the situation about deleting timeseries or storage group is relatively few. And we use `deviceIndexMap` when we initialize TsFileResource, when restarting or ingesting data, the performance seems more important. Maybe we need to make a balance here. Or we can delete useless string from `cachedStringPool` manually when time series is deleted instead of just using `intern()`.
What do you think?
[1] https://stackoverflow.com/questions/56203500/java-string-intern-use-hashtable-instead-of-concurrenthashmap
[2] https://www.fatalerrors.org/a/jvm-series-string.intern-performance-of.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] chengjianyun edited a comment on pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
chengjianyun edited a comment on pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#issuecomment-1022975116
> > For example, if we delete some storage groups or timeseries, some string would be never used but they are always kept in `cachedDevicePool` which cause memory leak.
>
> Hi, it's true that we can't handle the useless string now if we delete some devices. But the `intern()` method is not that fast[1][2] since the purpose of it is to save memory. By comparison, `concurrentHashMap` has a good performance. I believe that the situation about deleting timeseries or storage group is relatively few. And we use `deviceIndexMap` when we initialize TsFileResource, when restarting or ingesting data, the performance seems more important. Maybe we need to make a balance here. Or we can delete useless string from `cachedStringPool` manually when time series is deleted instead of just using `intern()`. What do you think?
>
> [1] https://stackoverflow.com/questions/56203500/java-string-intern-use-hashtable-instead-of-concurrenthashmap [2] https://www.fatalerrors.org/a/jvm-series-string.intern-performance-of.html
@Alima777, about performance of `String.intern()`, you can also refer [this blog](http://java-performance.info/string-intern-in-java-6-7-8/). I have verified the conclusion and got similar result on JDK8. So, it's better to set the `-XX:StringTableSize` when go bad perfromance on `intern()` method. And the performance improved much since JDK11, didn't verify on JDK9.
BTW, memory leak is intolerable in any cases especially in a database server system in my opinion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] chengjianyun commented on pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
chengjianyun commented on pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#issuecomment-1021956929
> Hi, is there any detailed reason about the memory leak risk?
`cachedDevicePool` never remove its key which is not reasonable. For example, if we delete some storage groups or timeseries, some string would be never used but they are always kept in `cachedDevicePool` which cause memory leak.
For string intern, JVM takes care of remove the useless string objects.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] chengjianyun commented on a change in pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
chengjianyun commented on a change in pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#discussion_r791320123
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/TsFileResource.java
##########
@@ -54,13 +54,7 @@
import java.nio.file.FileAlreadyExistsException;
import java.nio.file.Files;
import java.nio.file.Paths;
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
-import java.util.Random;
-import java.util.Set;
+import java.util.*;
Review comment:
Done.
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/VirtualStorageGroupProcessor.java
##########
@@ -45,14 +45,7 @@
import org.apache.iotdb.db.engine.upgrade.UpgradeLog;
import org.apache.iotdb.db.engine.version.SimpleFileVersionController;
import org.apache.iotdb.db.engine.version.VersionController;
-import org.apache.iotdb.db.exception.BatchProcessException;
-import org.apache.iotdb.db.exception.DiskSpaceInsufficientException;
-import org.apache.iotdb.db.exception.LoadFileException;
-import org.apache.iotdb.db.exception.StorageGroupProcessorException;
-import org.apache.iotdb.db.exception.TriggerExecutionException;
-import org.apache.iotdb.db.exception.TsFileProcessorException;
-import org.apache.iotdb.db.exception.WriteProcessException;
-import org.apache.iotdb.db.exception.WriteProcessRejectException;
+import org.apache.iotdb.db.exception.*;
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] chengjianyun edited a comment on pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
chengjianyun edited a comment on pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#issuecomment-1022975116
> > For example, if we delete some storage groups or timeseries, some string would be never used but they are always kept in `cachedDevicePool` which cause memory leak.
>
> Hi, it's true that we can't handle the useless string now if we delete some devices. But the `intern()` method is not that fast[1][2] since the purpose of it is to save memory. By comparison, `concurrentHashMap` has a good performance. I believe that the situation about deleting timeseries or storage group is relatively few. And we use `deviceIndexMap` when we initialize TsFileResource, when restarting or ingesting data, the performance seems more important. Maybe we need to make a balance here. Or we can delete useless string from `cachedStringPool` manually when time series is deleted instead of just using `intern()`. What do you think?
>
> [1] https://stackoverflow.com/questions/56203500/java-string-intern-use-hashtable-instead-of-concurrenthashmap [2] https://www.fatalerrors.org/a/jvm-series-string.intern-performance-of.html
@Alima777, about performance of `String.intern()`, you can also refer [this blog](http://java-performance.info/string-intern-in-java-6-7-8/). I have verified the conclusion and got similar result on JDK8. So, it's better to set the `-XX:StringTableSize` when go bad perfromance on `intern()` method. And the performance improved much since JDK11, didn't verify on JDK9.
BTW, memory leak is intolerable in any cases especially in a database server system in my opionion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] Alima777 commented on pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
Alima777 commented on pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#issuecomment-1022885986
> For example, if we delete some storage groups or timeseries, some string would be never used but they are always kept in `cachedDevicePool` which cause memory leak.
Hi, it's true that we can't handle the useless string if we delete some devices. But the intern() method is not that fast[1][2] since the purpose of it is to save memory. By comparison, `concurrentHashMap` has a good performance.
I believe that the situation about deleting timeseries or storage group is relatively few. And we use `deviceIndexMap` when we initialize TsFileResource, when restarting or ingesting data, the performance seems more important. Maybe we need to make a balance here. Or we can delete useless string from `cachedStringPool` manually when time series is deleted.
What do you think?
[1] https://stackoverflow.com/questions/56203500/java-string-intern-use-hashtable-instead-of-concurrenthashmap
[2] https://www.fatalerrors.org/a/jvm-series-string.intern-performance-of.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] coveralls commented on pull request #4964: Apply string intern for device String
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #4964:
URL: https://github.com/apache/iotdb/pull/4964#issuecomment-1019717716
[](https://coveralls.io/builds/45865605)
Coverage increased (+0.007%) to 67.74% when pulling **ad923c12e1b21f0c46c91f5180a7090af25d4a74 on chengjianyun:jianyun/intern** into **4e87d43d0bd18b30f96db9745bdef89c6ee1f4c1 on apache:master**.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org