gus-asf commented on code in PR #2708: URL: https://github.com/apache/solr/pull/2708#discussion_r1775480324
########## solr/core/src/java/org/apache/solr/search/MemAllowedLimit.java: ########## @@ -0,0 +1,130 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.search; + +import com.google.common.annotations.VisibleForTesting; +import java.lang.invoke.MethodHandles; +import java.lang.management.ManagementFactory; +import java.lang.management.ThreadMXBean; +import java.lang.reflect.Method; +import java.util.concurrent.atomic.AtomicLong; +import org.apache.solr.common.params.CommonParams; +import org.apache.solr.request.SolrQueryRequest; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class MemAllowedLimit implements QueryLimit { + private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); + private static final double MEBI = 1024.0 * 1024.0; + private static final ThreadMXBean threadBean = ManagementFactory.getThreadMXBean(); + private static Method GET_BYTES_METHOD; + private static boolean supported; + + static { + boolean testSupported; + try { + Class<?> sunThreadBeanClz = Class.forName("com.sun.management.ThreadMXBean"); + if (sunThreadBeanClz.isAssignableFrom(threadBean.getClass())) { + Method m = sunThreadBeanClz.getMethod("isThreadAllocatedMemorySupported"); + Boolean supported = (Boolean) m.invoke(threadBean); + if (supported) { + m = sunThreadBeanClz.getMethod("setThreadAllocatedMemoryEnabled", boolean.class); + m.invoke(threadBean, Boolean.TRUE); + testSupported = true; + GET_BYTES_METHOD = sunThreadBeanClz.getMethod("getCurrentThreadAllocatedBytes"); + } else { + testSupported = false; + } + } else { + testSupported = false; + } + } catch (Exception e) { + testSupported = false; + } + supported = testSupported; + } + + private static final ThreadLocal<AtomicLong> threadLocalMem = + ThreadLocal.withInitial(() -> new AtomicLong(-1L)); + + private long limitBytes; + private AtomicLong accumulatedMem = new AtomicLong(); + private long exitedAt = 0; + + public MemAllowedLimit(SolrQueryRequest req) { + if (!supported) { + throw new IllegalArgumentException( + "Per-thread memory allocation monitoring not available in this JVM."); + } + float reqMemLimit = req.getParams().getFloat(CommonParams.MEM_ALLOWED, -1.0f); + if (reqMemLimit <= 0.0f) { + throw new IllegalArgumentException( + "Check for limit with hasMemLimit(req) before creating a MemAllowedLimit!"); + } + limitBytes = Math.round(reqMemLimit * MEBI); + } + + @VisibleForTesting + MemAllowedLimit(float memLimit) { + limitBytes = Math.round(memLimit * MEBI); + } + + @VisibleForTesting + static boolean isSupported() { + return supported; + } + + static boolean hasMemLimit(SolrQueryRequest req) { + return req.getParams().getFloat(CommonParams.MEM_ALLOWED, -1.0f) > 0.0f; + } + + @Override + public boolean shouldExit() { + if (exitedAt > 0L) { + return true; + } + + try { + long currentAllocatedBytes = (Long) GET_BYTES_METHOD.invoke(threadBean); + AtomicLong threadMem = threadLocalMem.get(); + threadMem.compareAndExchange(-1L, currentAllocatedBytes); + long lastAllocatedBytes = threadMem.get(); + accumulatedMem.addAndGet(currentAllocatedBytes - lastAllocatedBytes); Review Comment: On the first pass this appears to be zero, meaning that you are ignoring bytes allocated before the first invocation of the limit check? Is that the intent? This class needs javadoc clearly stating what it intends to measure, so that future maintainers know what not to break (or if they want to break what you did on purpose). As it is I find it hard to evaluate the logic, and I am left shrugging and saying "I guess he meant to do that" ########## solr/solr-ref-guide/modules/query-guide/pages/common-query-parameters.adoc: ########## @@ -382,11 +382,20 @@ This parameter specifies the amount of CPU time, in milliseconds, allowed for a In contrast to the `timeAllowed` this parameter monitors the actual CPU usage by the thread that executes the query. The same CPU usage limit is applied to the query coordinator as to each replica that participates in the distributed search (although reaching this limit first in the query coordinator is unlikely). Should any replica locally exceed the allowed CPU time the whole distributed search will be terminated (by canceling requests to other shards). -Note: the same CPU limit is applied to each stage in the distributed query processing. Typically this involves two or more stages (e.g. getting top document id-s, retrieving their fields, additional stages may be required for faceting, grouping, etc). +Note: the same CPU limit is applied to each stage in the distributed query processing. Typically this involves two or more stages where the request is processed by different +Solr nodes (e.g. getting top document id-s, retrieving their fields, additional stages may be required for faceting, grouping, etc). For example, setting `cpuAllowed=500` gives a limit of at most 500 ms of CPU time for each of these stages - meaning that the total CPU usage by the query may reach a multiple of the `cpuAllowed` value depending on the number of stages. All other considerations regarding partial results listed for the `timeAllowed` parameter apply here, too. +== memAllowed Parameter + +This parameter specifies the amount of memory (a float value, in MiB) allowed for a search thread to allocate +during query execution. Similarly to the `cpuAllowed` this parameter monitors the additional memory +allocated to the thread that executes the current query. Similarly, the same hard limit is applied to +the query coordinator as to each replica that participates in the distributed search. As is the case with +the `cpuAllowed` also here the limit is applied separately to each stage of the distributed query processing. Review Comment: stage is a term not necessarily well defined from the reader's perspective. ########## solr/core/src/java/org/apache/solr/search/MemAllowedLimit.java: ########## @@ -0,0 +1,130 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.search; + +import com.google.common.annotations.VisibleForTesting; +import java.lang.invoke.MethodHandles; +import java.lang.management.ManagementFactory; +import java.lang.management.ThreadMXBean; +import java.lang.reflect.Method; +import java.util.concurrent.atomic.AtomicLong; +import org.apache.solr.common.params.CommonParams; +import org.apache.solr.request.SolrQueryRequest; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class MemAllowedLimit implements QueryLimit { + private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); + private static final double MEBI = 1024.0 * 1024.0; + private static final ThreadMXBean threadBean = ManagementFactory.getThreadMXBean(); + private static Method GET_BYTES_METHOD; + private static boolean supported; + + static { + boolean testSupported; + try { + Class<?> sunThreadBeanClz = Class.forName("com.sun.management.ThreadMXBean"); + if (sunThreadBeanClz.isAssignableFrom(threadBean.getClass())) { + Method m = sunThreadBeanClz.getMethod("isThreadAllocatedMemorySupported"); + Boolean supported = (Boolean) m.invoke(threadBean); + if (supported) { + m = sunThreadBeanClz.getMethod("setThreadAllocatedMemoryEnabled", boolean.class); + m.invoke(threadBean, Boolean.TRUE); + testSupported = true; + GET_BYTES_METHOD = sunThreadBeanClz.getMethod("getCurrentThreadAllocatedBytes"); + } else { + testSupported = false; + } + } else { + testSupported = false; + } + } catch (Exception e) { + testSupported = false; + } + supported = testSupported; + } + + private static final ThreadLocal<AtomicLong> threadLocalMem = + ThreadLocal.withInitial(() -> new AtomicLong(-1L)); + + private long limitBytes; + private AtomicLong accumulatedMem = new AtomicLong(); + private long exitedAt = 0; + + public MemAllowedLimit(SolrQueryRequest req) { + if (!supported) { + throw new IllegalArgumentException( + "Per-thread memory allocation monitoring not available in this JVM."); + } + float reqMemLimit = req.getParams().getFloat(CommonParams.MEM_ALLOWED, -1.0f); + if (reqMemLimit <= 0.0f) { + throw new IllegalArgumentException( + "Check for limit with hasMemLimit(req) before creating a MemAllowedLimit!"); + } + limitBytes = Math.round(reqMemLimit * MEBI); + } + + @VisibleForTesting + MemAllowedLimit(float memLimit) { + limitBytes = Math.round(memLimit * MEBI); + } + + @VisibleForTesting + static boolean isSupported() { + return supported; + } + + static boolean hasMemLimit(SolrQueryRequest req) { + return req.getParams().getFloat(CommonParams.MEM_ALLOWED, -1.0f) > 0.0f; + } + + @Override + public boolean shouldExit() { + if (exitedAt > 0L) { + return true; + } + + try { + long currentAllocatedBytes = (Long) GET_BYTES_METHOD.invoke(threadBean); + AtomicLong threadMem = threadLocalMem.get(); + threadMem.compareAndExchange(-1L, currentAllocatedBytes); Review Comment: compareAndSet is a more common method with an intuitive name that the reader may not have to look up. Since no return is checked here it would be sufficient. ########## solr/core/src/java/org/apache/solr/search/MemAllowedLimit.java: ########## @@ -0,0 +1,130 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.search; + +import com.google.common.annotations.VisibleForTesting; +import java.lang.invoke.MethodHandles; +import java.lang.management.ManagementFactory; +import java.lang.management.ThreadMXBean; +import java.lang.reflect.Method; +import java.util.concurrent.atomic.AtomicLong; +import org.apache.solr.common.params.CommonParams; +import org.apache.solr.request.SolrQueryRequest; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class MemAllowedLimit implements QueryLimit { + private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); + private static final double MEBI = 1024.0 * 1024.0; + private static final ThreadMXBean threadBean = ManagementFactory.getThreadMXBean(); + private static Method GET_BYTES_METHOD; + private static boolean supported; Review Comment: final? ########## solr/solr-ref-guide/modules/query-guide/pages/common-query-parameters.adoc: ########## @@ -382,11 +382,20 @@ This parameter specifies the amount of CPU time, in milliseconds, allowed for a In contrast to the `timeAllowed` this parameter monitors the actual CPU usage by the thread that executes the query. The same CPU usage limit is applied to the query coordinator as to each replica that participates in the distributed search (although reaching this limit first in the query coordinator is unlikely). Should any replica locally exceed the allowed CPU time the whole distributed search will be terminated (by canceling requests to other shards). -Note: the same CPU limit is applied to each stage in the distributed query processing. Typically this involves two or more stages (e.g. getting top document id-s, retrieving their fields, additional stages may be required for faceting, grouping, etc). +Note: the same CPU limit is applied to each stage in the distributed query processing. Typically this involves two or more stages where the request is processed by different +Solr nodes (e.g. getting top document id-s, retrieving their fields, additional stages may be required for faceting, grouping, etc). For example, setting `cpuAllowed=500` gives a limit of at most 500 ms of CPU time for each of these stages - meaning that the total CPU usage by the query may reach a multiple of the `cpuAllowed` value depending on the number of stages. All other considerations regarding partial results listed for the `timeAllowed` parameter apply here, too. +== memAllowed Parameter + +This parameter specifies the amount of memory (a float value, in MiB) allowed for a search thread to allocate +during query execution. Similarly to the `cpuAllowed` this parameter monitors the additional memory +allocated to the thread that executes the current query. Similarly, the same hard limit is applied to Review Comment: Similarly used twice... which is strange (bad writing style) and that leaves me wondering if there's something edited incorrectly here? Also I'm not sure if comparing it to `cpuAllowed` is helpful here. Might be clearer to just describe what it does directly. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
