RFR: 8315373: Change VirtualThread to unmount after freezing, re-mount before thawing
In the virtual thread implementation, thread identity switches to the carrier before freezing and switches back to the virtual thread after thawing. This was a forced move due to issues getting JVMTI to work with virtual threads. JVMTI can now hide events during transitions so we can invert the sequence back to mounting before running the continuation, unmounting after freezing, and re-mounting after thawing. This sequence is important for future changes that will initiate the freezing from the VM. The change requires an update to the JFR thread sampler to skip sampling when it samples during a transition. Testing: tier1-5 - Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/15492/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15492&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8315373 Stats: 45 lines in 2 files changed: 10 ins; 16 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/15492.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15492/head:pull/15492 PR: https://git.openjdk.org/jdk/pull/15492
RFR: 8315437: Enable parallelism in vmTestbase/nsk/monitoring/stress/classload tests
Current vmTestbase/nsk/monitoring/stress/classload tests contains 24 tests, each running exclusively. This drags the tier4 test times up. There seem to be no reason to run these tests exclusively, though: they complete in reasonable time, are lightly-threaded, and consume the usual amount of memory. We should consider enabling parallelism for them and get improved test performance. Currently it is blocked by TEST.properties with exclusiveAccess.dirs directives in them. Current run on 18-core machine: 2011.95s user 201.83s system 356% cpu 10:20.19 total Fully parallel: 2274.02s user 128.64s system 2029% cpu 1:58.39 total Additional testing: - [ ] 100x iterations of Linux x86_64 fastdebug `vmTestbase/nsk/monitoring/stress/classload` - Commit messages: - Fix Changes: https://git.openjdk.org/jdk/pull/15506/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15506&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8315437 Stats: 552 lines in 24 files changed: 0 ins; 552 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15506.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15506/head:pull/15506 PR: https://git.openjdk.org/jdk/pull/15506
RFR: 8315442: Enable parallelism in vmTestbase/nsk/monitoring/stress/thread tests
Current vmTestbase/nsk/monitoring/stress/thread tests contains 21 tests, each running exclusively. This drags the tier4 test times up. There seem to be no reason to run these tests exclusively, though: they complete in reasonable time, are lightly-threaded, and consume the usual amount of memory. We should consider enabling parallelism for them and get improved test performance. Currently it is blocked by TEST.properties with exclusiveAccess.dirs directives in them. Current run on 18-core machine: 1025.63s user 97.23s system 586% cpu 3:11.60 total Fully parallel: 1125.04s user 107.60s system 1583% cpu 1:17.83 total Additional testing: - [x] 50x iterations of Linux x86_64 fastdebug `vmTestbase/nsk/monitoring/stress/thread` - Commit messages: - Fix Changes: https://git.openjdk.org/jdk/pull/15508/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15508&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8315442 Stats: 483 lines in 21 files changed: 0 ins; 483 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15508.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15508/head:pull/15508 PR: https://git.openjdk.org/jdk/pull/15508
Re: RFR: 8315097: Rename createJavaProcessBuilder [v3]
On Thu, 31 Aug 2023 05:45:27 GMT, David Holmes wrote: > > So you could create a single createJavaProcessBuilder with add an > > additional parameter boolean addTestOpts e.g. > > createJavaProcessBuilder(List command, boolean addTestOpts) { ... } > > @msheppar that is actually where we started, and it was then split into two > differently named methods to "make it clear" which one included the test opts > without having to remember the name of the parameter that the true/false > argument was bound to. cheers David thanks for the clarification on background - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1700749844
Re: RFR: 8315373: Change VirtualThread to unmount after freezing, re-mount before thawing
On Wed, 30 Aug 2023 13:56:42 GMT, Alan Bateman wrote: > In the virtual thread implementation, thread identity switches to the carrier > before freezing and switches back to the virtual thread after thawing. This > was a forced move due to issues getting JVMTI to work with virtual threads. > JVMTI can now hide events during transitions so we can invert the sequence > back to mounting before running the continuation, unmounting after freezing, > and re-mounting after thawing. This sequence is important for future changes > that will initiate the freezing from the VM. > > The change requires an update to the JFR thread sampler to skip sampling when > it samples during a transition. > > Testing: tier1-5 src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp line 410: > 408: } > 409: if (JAVA_SAMPLE == type) { > 410: if (thread_state_in_java(thread) && > !is_vthread_in_transition(thread)) { I think this check can be postponed until after the thread is suspended. src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp line 415: > 413: } else { > 414: assert(NATIVE_SAMPLE == type, "invariant"); > 415: if (thread_state_in_native(thread) && > !is_vthread_in_transition(thread)) { Is this possible? I assume the thread is in _thread_in_Java during the transition? - PR Review Comment: https://git.openjdk.org/jdk/pull/15492#discussion_r1311439052 PR Review Comment: https://git.openjdk.org/jdk/pull/15492#discussion_r1311437865
Re: RFR: 8315373: Change VirtualThread to unmount after freezing, re-mount before thawing
On Thu, 31 Aug 2023 10:38:32 GMT, Markus Grönlund wrote: >> In the virtual thread implementation, thread identity switches to the >> carrier before freezing and switches back to the virtual thread after >> thawing. This was a forced move due to issues getting JVMTI to work with >> virtual threads. JVMTI can now hide events during transitions so we can >> invert the sequence back to mounting before running the continuation, >> unmounting after freezing, and re-mounting after thawing. This sequence is >> important for future changes that will initiate the freezing from the VM. >> >> The change requires an update to the JFR thread sampler to skip sampling >> when it samples during a transition. >> >> Testing: tier1-5 > > src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp line 410: > >> 408: } >> 409: if (JAVA_SAMPLE == type) { >> 410: if (thread_state_in_java(thread) && >> !is_vthread_in_transition(thread)) { > > I think this check can be postponed until after the thread is suspended. Is the check in OSThreadSampler::protected_task in the right place? That seems to be a suspended context. For JfrThreadSampleClosure::do_sample_thread, I think I may have mis-read the code. I thought it was suspended but looking at it again them maybe it should be the JfrNativeSamplerCallback implementation. This is probably an area where I need help to get right. Is JfrNativeSamplerCallback - PR Review Comment: https://git.openjdk.org/jdk/pull/15492#discussion_r1311502783
Re: RFR: 8309271: A way to align already compiled methods with compiler directives [v6]
> Compiler Control (https://openjdk.org/jeps/165) provides method-context > dependent control of the JVM compilers (C1 and C2). The active directive > stack is built from the directive files passed with the > `-XX:CompilerDirectivesFile` diagnostic command-line option and the > Compiler.add_directives diagnostic command. It is also possible to clear all > directives or remove the top from the stack. > > A matching directive will be applied at method compilation time when such > compilation is started. If directives are added or changed, but compilation > does not start, then the state of compiled methods doesn't correspond to the > rules. This is not an error, and it happens in long running applications when > directives are added or removed after compilation of methods that could be > matched. For example, the user decides that C2 compilation needs to be > disabled for some method due to a compiler bug, issues such a directive but > this does not affect the application behavior. In such case, the target > application needs to be restarted, and such an operation can have high costs > and risks. Another goal is testing/debugging compilers. > > It would be convenient to optionally reconcile at least existing matching > nmethods to the current stack of compiler directives (so bypass inlined > methods). > > Natural way to eliminate the discrepancy between the result of compilation > and the broken rule is to discard the compilation result, i.e. > deoptimization. Prior to that we can try to re-compile the method letting > compile broker to perform it taking new directives stack into account. > Re-compilation helps to prevent hot methods from execution in the interpreter. > > A new flag `-r` has beed introduced for some directives related to compile > commands: `Compiler.add_directives`, `Compiler.remove_directives`, > `Compiler.clear_directives`. The default behavior has not changed (no flag). > If the new flag is present, the command scans already compiled methods and > puts methods that have any active non-default matching compiler directives to > re-compilation if possible, otherwise marks them for deoptimization. There is > currently no distinction which directives are found. In particular, this > means that if there are rules for inlining into some method, it will be > refreshed. On the other hand, if there are rules for a method and it was > inlined, top-level methods won't be refreshed, but this can be achieved by > having rules for them. > > In addition, a new diagnostic command `Compiler.replace_directives`, has been > added for ... Dmitry Chuyko has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 24 commits: - Merge branch 'openjdk:master' into compiler-directives-force-update - jcheck - Unnecessary import - force_update->refresh - Merge branch 'openjdk:master' into compiler-directives-force-update - Use only top directive for add/remove; better mutex rank definition; texts - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Safe handling of non-java methods - Merge branch 'openjdk:master' into compiler-directives-force-update - ... and 14 more: https://git.openjdk.org/jdk/compare/145d8bc1...b4eb6de0 - Changes: https://git.openjdk.org/jdk/pull/14111/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14111&range=05 Stats: 372 lines in 15 files changed: 339 ins; 3 del; 30 mod Patch: https://git.openjdk.org/jdk/pull/14111.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14111/head:pull/14111 PR: https://git.openjdk.org/jdk/pull/14111
Re: RFR: 8315373: Change VirtualThread to unmount after freezing, re-mount before thawing
On Thu, 31 Aug 2023 11:41:03 GMT, Alan Bateman wrote: >> src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp line 410: >> >>> 408: } >>> 409: if (JAVA_SAMPLE == type) { >>> 410: if (thread_state_in_java(thread) && >>> !is_vthread_in_transition(thread)) { >> >> I think this check can be postponed until after the thread is suspended. > > Is the check in OSThreadSampler::protected_task in the right place? That > seems to be a suspended context. > > For JfrThreadSampleClosure::do_sample_thread, I think I may have mis-read the > code. I thought it was suspended but looking at it again them maybe it should > be the JfrNativeSamplerCallback implementation. This is probably an area > where I need help to get right. OSThreadSampler::protected_task() is the correct place to perform the test. The sampled thread is completely suspended at this location. The other locations are optimistic, they read the thread state while the sampled thread is running to avoid signalling a thread unnecessarily. You don't need the is_vthread_in_transtition() check there, especially not for the thread_in_native case, since that would not be possible, would it? - PR Review Comment: https://git.openjdk.org/jdk/pull/15492#discussion_r1311632129
Re: RFR: 8315442: Enable parallelism in vmTestbase/nsk/monitoring/stress/thread tests
On Thu, 31 Aug 2023 08:40:58 GMT, Aleksey Shipilev wrote: > Current vmTestbase/nsk/monitoring/stress/thread tests contains 21 tests, each > running exclusively. This drags the tier4 test times up. There seem to be no > reason to run these tests exclusively, though: they complete in reasonable > time, are lightly-threaded, and consume the usual amount of memory. > > We should consider enabling parallelism for them and get improved test > performance. Currently it is blocked by TEST.properties with > exclusiveAccess.dirs directives in them. > > Current run on 18-core machine: > 1025.63s user 97.23s system 586% cpu 3:11.60 total > > Fully parallel: > 1125.04s user 107.60s system 1583% cpu 1:17.83 total > > Additional testing: > - [x] 50x iterations of Linux x86_64 fastdebug > `vmTestbase/nsk/monitoring/stress/thread` I think this is fine, but it may inconvenience some test maintainers since we may now run against /proc/sys/kernel/threads-max or whatever other limits there are. Or do we do worse things already in other tests? - Marked as reviewed by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15508#pullrequestreview-1604843845
Re: RFR: 8315442: Enable parallelism in vmTestbase/nsk/monitoring/stress/thread tests
On Thu, 31 Aug 2023 08:40:58 GMT, Aleksey Shipilev wrote: > Current vmTestbase/nsk/monitoring/stress/thread tests contains 21 tests, each > running exclusively. This drags the tier4 test times up. There seem to be no > reason to run these tests exclusively, though: they complete in reasonable > time, are lightly-threaded, and consume the usual amount of memory. > > We should consider enabling parallelism for them and get improved test > performance. Currently it is blocked by TEST.properties with > exclusiveAccess.dirs directives in them. > > Current run on 18-core machine: > 1025.63s user 97.23s system 586% cpu 3:11.60 total > > Fully parallel: > 1125.04s user 107.60s system 1583% cpu 1:17.83 total > > Additional testing: > - [x] 50x iterations of Linux x86_64 fastdebug > `vmTestbase/nsk/monitoring/stress/thread` Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15508#pullrequestreview-1605021282
Re: RFR: 8313277: Resolve multiple definition of 'normalize' when statically linking JDK native libraries with user code
On Thu, 31 Aug 2023 04:53:26 GMT, David Holmes wrote: > No not namespaces, just by using C++ so it a member function, then C++ name > mangling would mean the symbol would be different to some same-named function > in some other library - in essence the class name part of the symbol would > provide the "unique" prefix. Thanks for the clarification. Yeah, C++ name mangling indeed helps in many cases. For hotspot code, we've only seen very few issues where the class names are the same, such as StringTable and Thread (with pending OpenJDK bugs filed already). - PR Comment: https://git.openjdk.org/jdk/pull/15477#issuecomment-1701292028
Re: RFR: 8313277: Resolve multiple definition of 'normalize' when statically linking JDK native libraries with user code
On Thu, 31 Aug 2023 05:00:18 GMT, David Holmes wrote: > Is `normalize` the only function that causes a conflict and the other changes > are just for consistency? If so please just rename normalize to e.g. > `normalize_path` Right, only `normalize` showed up in our testing so far. I suggested @cjmoon1 to also change other functions in the file mainly for consistency purpose. Just rename normalize to `normalize_path` sounds ok to me. Will wait a bit to see if @sspitsyn has any thoughts as well. - PR Comment: https://git.openjdk.org/jdk/pull/15477#issuecomment-1701300310
Re: RFR: 8313277: Resolve multiple definition of 'normalize' when statically linking JDK native libraries with user code
On Tue, 29 Aug 2023 23:37:06 GMT, Jiangli Zhou wrote: > Please review this simple change from @cjmoon1 for resolving static linking > issue caused by multiple definition of 'normalize'. @cjmoon1's branch can be > found at: > https://github.com/openjdk/jdk/compare/master...cjmoon1:jdk:fix_normalize. > > The change incorporated suggestions from both @sspitsyn and myself: > > - > https://bugs.openjdk.org/browse/JDK-8313277?focusedCommentId=14604187&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14604187 > - > https://bugs.openjdk.org/browse/JDK-8313277?focusedCommentId=14604187&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14604187 I'm okay to fix just `normalize()`. Converting all JDWP agent files to C++ looks like a major effort. - PR Comment: https://git.openjdk.org/jdk/pull/15477#issuecomment-1701428685
Re: RFR: 8313277: Resolve multiple definition of 'normalize' when statically linking JDK native libraries with user code
On Tue, 29 Aug 2023 23:37:06 GMT, Jiangli Zhou wrote: > Please review this simple change from @cjmoon1 for resolving static linking > issue caused by multiple definition of 'normalize'. @cjmoon1's branch can be > found at: > https://github.com/openjdk/jdk/compare/master...cjmoon1:jdk:fix_normalize. > > The change incorporated suggestions from both @sspitsyn and myself: > > - > https://bugs.openjdk.org/browse/JDK-8313277?focusedCommentId=14604187&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14604187 > - > https://bugs.openjdk.org/browse/JDK-8313277?focusedCommentId=14604187&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14604187 That sounds good to me as well. I'll go ahead and make the change. - PR Comment: https://git.openjdk.org/jdk/pull/15477#issuecomment-1701433188
Re: RFR: 8315373: Change VirtualThread to unmount after freezing, re-mount before thawing
On Wed, 30 Aug 2023 13:56:42 GMT, Alan Bateman wrote: > In the virtual thread implementation, thread identity switches to the carrier > before freezing and switches back to the virtual thread after thawing. This > was a forced move due to issues getting JVMTI to work with virtual threads. > JVMTI can now hide events during transitions so we can invert the sequence > back to mounting before running the continuation, unmounting after freezing, > and re-mounting after thawing. This sequence is important for future changes > that will initiate the freezing from the VM. > > The change requires an update to the JFR thread sampler to skip sampling when > it samples during a transition. > > Testing: tier1-5 Looks good to me. - Marked as reviewed by pchilanomate (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15492#pullrequestreview-1605291739
Re: RFR: 8315373: Change VirtualThread to unmount after freezing, re-mount before thawing
On Thu, 31 Aug 2023 10:37:26 GMT, Markus Grönlund wrote: >> In the virtual thread implementation, thread identity switches to the >> carrier before freezing and switches back to the virtual thread after >> thawing. This was a forced move due to issues getting JVMTI to work with >> virtual threads. JVMTI can now hide events during transitions so we can >> invert the sequence back to mounting before running the continuation, >> unmounting after freezing, and re-mounting after thawing. This sequence is >> important for future changes that will initiate the freezing from the VM. >> >> The change requires an update to the JFR thread sampler to skip sampling >> when it samples during a transition. >> >> Testing: tier1-5 > > src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp line 415: > >> 413: } else { >> 414: assert(NATIVE_SAMPLE == type, "invariant"); >> 415: if (thread_state_in_native(thread) && >> !is_vthread_in_transition(thread)) { > > Is this possible? I assume the thread is in _thread_in_Java during the > transition? There are some native methods that we execute during mount/unmount transitions. From what I see they all seem to be defined as `@IntrinsicCandidate`, but if for some reason we don't execute the intrinsic version (interp only mode for instance) then we would call a normal native method. - PR Review Comment: https://git.openjdk.org/jdk/pull/15492#discussion_r1311970316
Integrated: JDK-8313656: assert(!JvmtiExport::can_support_virtual_threads()) with -XX:-DoJVMTIVirtualThreadTransitions
On Thu, 10 Aug 2023 01:45:47 GMT, Alex Menkov wrote: > The change fixes several issues with capability management: > - handling can_support_virtual_threads capability. > JvmtiExport::can_support_virtual_threads() should be set to true if we have > one or more agent with can_support_virtual_threads capability; > - JvmtiManageCapabilities (used by > GetPotentialCapabilities/AddCapabilities/RelinquishCapabilities/GetCapabilities) > is not thread safe; > - SuspendAllVirtualThreads and ResumeAllVirtualThreads should check > capability on the caller agent (and not > JvmtiExport::can_support_virtual_threads()). > > Testing: tier1..5 This pull request has now been integrated. Changeset: b38bcae1 Author:Alex Menkov URL: https://git.openjdk.org/jdk/commit/b38bcae1bad399d0a3ffc091835bf89140550bc2 Stats: 53 lines in 3 files changed: 47 ins; 0 del; 6 mod 8313656: assert(!JvmtiExport::can_support_virtual_threads()) with -XX:-DoJVMTIVirtualThreadTransitions Reviewed-by: sspitsyn, lmesnik - PR: https://git.openjdk.org/jdk/pull/15219
Re: RFR: 8315373: Change VirtualThread to unmount after freezing, re-mount before thawing
On Thu, 31 Aug 2023 17:31:34 GMT, Patricio Chilano Mateo wrote: >> src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp line 415: >> >>> 413: } else { >>> 414: assert(NATIVE_SAMPLE == type, "invariant"); >>> 415: if (thread_state_in_native(thread) && >>> !is_vthread_in_transition(thread)) { >> >> Is this possible? I assume the thread is in _thread_in_Java during the >> transition? > > There are some native methods that we execute during mount/unmount > transitions. From what I see they all seem to be defined as > `@IntrinsicCandidate`, but if for some reason we don't execute the intrinsic > version (interp only mode for instance) then we would call a normal native > method. Just to ad that Patricio suggested today to run the stress tests with -Xint and that does lead to triggered the assert quickly when the thread is sampled in native. There are several native methods that are @IntrinsicCandidate that are invoked after the thread identity has changed to the virtual thread and before the continuation is mounted. Probably less likely on the unmount as the only native method is the one that reverts the thread identity. - PR Review Comment: https://git.openjdk.org/jdk/pull/15492#discussion_r1312128046
Re: RFR: 8311775: [TEST] duplicate verifyHeapDump in several tests [v4]
On Thu, 31 Aug 2023 01:53:36 GMT, Yi Yang wrote: >> This is a follow-up patch of #13667. verifyHeapDump is duplicated in several >> tests, this patch tries to consolidate them into one method. > > Yi Yang has updated the pull request incrementally with one additional commit > since the last revision: > > Update HeapDumpTest.java test/lib/jdk/test/lib/hprof/HprofParser.java needs a copyright update. - PR Comment: https://git.openjdk.org/jdk/pull/15202#issuecomment-1701667003
Re: RFR: 8311775: [TEST] duplicate verifyHeapDump in several tests [v5]
> This is a follow-up patch of #13667. verifyHeapDump is duplicated in several > tests, this patch tries to consolidate them into one method. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: Update HprofParser.java - Changes: - all: https://git.openjdk.org/jdk/pull/15202/files - new: https://git.openjdk.org/jdk/pull/15202/files/2408e194..a883e746 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15202&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15202&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15202.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15202/head:pull/15202 PR: https://git.openjdk.org/jdk/pull/15202
Re: RFR: 8315442: Enable parallelism in vmTestbase/nsk/monitoring/stress/thread tests
On Thu, 31 Aug 2023 08:40:58 GMT, Aleksey Shipilev wrote: > Current vmTestbase/nsk/monitoring/stress/thread tests contains 21 tests, each > running exclusively. This drags the tier4 test times up. There seem to be no > reason to run these tests exclusively, though: they complete in reasonable > time, are lightly-threaded, and consume the usual amount of memory. > > We should consider enabling parallelism for them and get improved test > performance. Currently it is blocked by TEST.properties with > exclusiveAccess.dirs directives in them. > > Current run on 18-core machine: > 1025.63s user 97.23s system 586% cpu 3:11.60 total > > Fully parallel: > 1125.04s user 107.60s system 1583% cpu 1:17.83 total > > Additional testing: > - [x] 50x iterations of Linux x86_64 fastdebug > `vmTestbase/nsk/monitoring/stress/thread` There is no explanation as to why these were run exclusively when first introduced, unfortunately. I'm always concerned about running multiple "stress" tests concurrently however. @lmesnik can you test this in our CI please? (I would but am starting vacation in a few hours). - PR Review: https://git.openjdk.org/jdk/pull/15508#pullrequestreview-1606134236